Change manifest.schema.json to apply by reference a loose-checks manifest schema and a strict-checks manifest schema, then use these to perform validation#199
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe pull request restructures manifest validation by splitting a monolithic schema into two composable schemas: a loose-manifest schema defining the base structure and a strict-refinements schema enforcing content validation. The manifest.ts and schema-compile.mjs files are updated to support dual-mode validation (loose and strict), with new TypeScript type exports added for LooseModuleManifest. Changes
Poem
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/companion-module-base/src/manifest.ts (1)
14-16: Add a defensive guard before invoking the selected validator.Line 14-Line 16 can fail with a generic TypeError if the generated validator map shape changes. A tiny guard gives a clearer failure mode.
Suggested change
function tryValidation(type: keyof typeof validations, manifest: LooseModuleManifest) { const validateManifestSchema = validations[type] + if (typeof validateManifestSchema !== 'function') { + throw new Error(`Manifest validator '${String(type)}' is not available`) + } if (!validateManifestSchema(manifest)) { const errors = validateManifestSchema.errors if (!errors) throw new Error(`Manifest failed validation with unknown reason`)
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/companion-module-base/assets/loose-manifest.schema.jsonpackages/companion-module-base/assets/manifest.schema.jsonpackages/companion-module-base/assets/strict-refinements.schema.jsonpackages/companion-module-base/src/manifest.tspackages/companion-module-base/tools/schema-compile.mjs
|
I'm trying to work through the bot's exports complaint, and I can't actually tell whether it's right or wrong. Things were working correctly for me locally, but I'm not sure if this is a function of prior efforts on the pre-2.0 and monorepo changes -- which seem to make complaint about |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/companion-module-base/tools/schema-compile.mjs (1)
20-23:⚠️ Potential issue | 🔴 CriticalPotential AJV multi-schema ESM export mismatch with runtime default import.
Line 20-23 generates multiple validators; with AJV standalone ESM this is typically named exports.
packages/companion-module-base/src/manifest.tscurrently default-importsvalidate_manifest.js, so this can fail at runtime if no default export is emitted.#!/bin/bash set -euo pipefail echo "== validate_manifest.js export shape ==" if [[ -f packages/companion-module-base/generated/validate_manifest.js ]]; then rg -n "export default|export const ModuleManifest|export const LooseModuleManifest" \ packages/companion-module-base/generated/validate_manifest.js else echo "generated/validate_manifest.js not found" fi echo "" echo "== runtime import usage in manifest.ts ==" rg -n "import .*validate_manifest\\.js|validations\\[type\\]|tryValidation\\(" \ packages/companion-module-base/src/manifest.tsExpected result: either (a) generated file includes
export defaultmatching current import style, or (b) runtime code is switched to named imports.🔧 Suggested fix (wrap named exports into a default object)
const moduleCode = standaloneCode(ajv, { ModuleManifest: '/assets/manifest.schema.json', LooseModuleManifest: '/assets/loose-manifest.schema.json', }) +const wrappedModuleCode = `${moduleCode} + +const validations = { ModuleManifest, LooseModuleManifest } +export default validations +` // Now you can write the module code to file const outputPath = new URL('../generated/validate_manifest.js', import.meta.url) -fs.writeFileSync(outputPath, moduleCode) +fs.writeFileSync(outputPath, wrappedModuleCode)
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/companion-module-base/assets/loose-manifest.schema.jsonpackages/companion-module-base/assets/manifest.schema.jsonpackages/companion-module-base/assets/strict-refinements.schema.jsonpackages/companion-module-base/src/manifest.tspackages/companion-module-base/tools/schema-compile.mjs
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/companion-module-base/assets/loose-manifest.schema.json (1)
203-205: Consider using"integer"for portPorts are whole numbers (1-65535), so using
"integer"instead of"number"would prevent fractional values from passing validation. Totally optional though - just a small refinement if you want to be extra precise!"port": { - "type": "number" + "type": "integer" },
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 46a60e15-ab45-4875-8940-bdee54eda405
📒 Files selected for processing (5)
packages/companion-module-base/assets/loose-manifest.schema.jsonpackages/companion-module-base/assets/manifest.schema.jsonpackages/companion-module-base/assets/strict-refinements.schema.jsonpackages/companion-module-base/src/manifest.tspackages/companion-module-base/tools/schema-compile.mjs
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/companion-module-base/assets/manifest.schema.json
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/base/assets/loose-manifest.schema.json (1)
26-29:⚠️ Potential issue | 🟡 MinorSmall typo: trailing space in description
There's a trailing space at the end of the description text on line 28. This was flagged in a previous review but appears to still be present.
"description": { "type": "string", - "description": "Description of the module " + "description": "Description of the module" },
🧹 Nitpick comments (2)
packages/base/assets/loose-manifest.schema.json (1)
154-171: Using empty string for patternProperties works, but consider documenting itUsing
""as the pattern inpatternPropertieseffectively matches all property names, which seems intentional for allowing arbitrary bonjour query names. This is valid JSON Schema, though it's a bit unconventional.If you wanted to be more explicit about the pattern (for readability), you could use
".*"or"^.*$", but the empty string works just fine! Just mentioning in case future maintainers wonder about it. 😊packages/base/src/manifest.ts (1)
17-26: Clean validation helper! ✨The
tryValidationfunction nicely centralizes error formatting. One small suggestion for the type definition - you might consider makingerrorsmore specific if you want better type safety:type ValidateFunc = ((manifest: LooseModuleManifest) => boolean) & { errors: Array<{ instancePath: string; message?: string }> | null }But honestly,
unknownworks fine here since you're just JSON-stringifying the errors anyway. Feel free to keep it as-is! 😊
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3edc931d-301e-4f2b-92c3-74eeeb7d9138
📒 Files selected for processing (5)
packages/base/assets/loose-manifest.schema.jsonpackages/base/assets/manifest.schema.jsonpackages/base/assets/strict-refinements.schema.jsonpackages/base/src/manifest.tspackages/base/tools/schema-compile.mjs
jswalden
force-pushed
the
manifest-fu
branch
3 times, most recently
from
a0a8973 to
8fa07d8
Compare
|
I'm wondering if instead of having to maintain a strict and a loose schema, maybe we just write the strict one and we generate the loose one from that? It appears as though we only use the loose one for generating the loose ajv validator, so we wouldn't need the file which does the allof. While in some ways this split file is simpler (less automation), it introduces risk of drift, and makes for more complicated schema files. As a bonus it will hopefully nudge me to add validation for new things too, as it will be in the one file (and hoepfulyl coderabbit will flag the missing patterns/whatever) What do you think about that? |
…est.schema.json`.
…eference `assets/loose-manifest.schema.json`.
…ON schemas that modules can use in their own manifests. (bitfocus#198)
It's doable in schema-unaware fashion. You'd have to assume that But you also wouldn't stop at Also, doing it the way this currently does it means you get to share a bunch of generated validation code. Munging and compiling two separate schemas will duplicate the code to validate most of the two schemas. An This feels like enough playing fast and loose to make me leery about the technique.
My first hope was schemas could be parametrized, so you could define one schema, then instantiate it twice with parametrically different semantics. But JSON Schema doesn't work that way. A parallel overlay of restrictions was the best in-system approach. Or, we could do the parametrizing in JS -- move the manifest schema into JS directly, then return variants of it from a function that takes a
Like I said, I'm leery. Having actually done some of the work experimenting with these approaches, and considering the extra validation code concern of the alternatives to this, I think my approach here is maybe the least bad option of the lot. |
I'm not too worried about duplication here. While the file is 88kb, its not a horrible volume to duplicate. Modules won't be including it, so this would only affect companion. My main worry is about DX for maintaining the schemas, to reduce the risk of errors in something that gets changed rarely so will be easy to forget steps for. But perhaps that drift/stricter validation could also be verified with a unit test to enforce the two schema files align? I suppose that another approach entirely would be to replace the ajv validator with something else. If we controlled the code generation there, then one validator function could handle both modes. |
We could also have something akin to (vaguely, handwaving) function generateManifestSchemas() {
function generateSchema(loose) {
const schemaTitle = loose ? 'LooseModuleManifest' : 'ModuleManifest'
const pattern = (pat) => loose ? {} : { pattern: pat }
return {
title: schemaTitle,
properties: {
id: {
type: 'string',
...pattern('^((?!companion-module-your-module-name).)*$'),
},
}
}
}
const looseSchema = generateSchema(true)
const schema = generateSchema(false)
return [looseSchema, schema]
}and then do the parametrizing in JS. The "downside", such as it is, is that the schema wouldn't be a standalone JSON file any more. Does that potentially sound preferable? (I actually started working on this the other day, then it spiraled far enough that it became not worth doing without some indication of being desirable.) |
|
I was meaning to keep the strict json schema file, but replace the generated validator function from ajv with a custom validator function. I do think it important to keep a schema file on disk in the npm package, but that doesn't mean we couldn't generate it from code. In the same way we generate the validator and ts types today, the same script could write things to disk So yeah I think the best options currently are that generation proposal of yours, or to merge this pr as is and rely on some not-yet written unit tests to ensure the schemas remain in sync and both get updated |
|
I did a bunch more working on doing all schema processing programmatically. It gets substantially more involved than what is actually here. Having separately-defined schemas means you now have not just one root of the schema tree, but two. Handling of this starts requiring defining resolvers so things can match up.
And then, if you want to ship the schemas in Actually generating this stuff, is much worse than just using a few flat files as done here. https://github.com/jswalden/companion-module-base/tree/manifest-fu-strip-patterns exists but doesn't handle the generate-two-separate-files thing (or properly generating a flat schema into |
2 participants
Worth noting that I did this in three revs to clarify that the file-move didn't change the file, then to make the smaller changes to the moved file readable. You may (or may not) want to squash these into one final commit, since there's to-ing and fro-ing here that clarifies the changes now but might not be as good for long-term readability.
Summary by CodeRabbit
New Features
Refactor