-
Notifications
You must be signed in to change notification settings - Fork 289
Require JSON support for new commands #8406
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
gonzaloriestra
wants to merge
1
commit into
gonzalo/json-error-handling-v2
from
gonzalo/json-support-by-default
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,6 +6,11 @@ keep running and streaming updates, such as `shopify app dev`, are outside this | |
| Finite commands expose their successful result as typed data independently from terminal presentation. The command's | ||
| domain package owns this contract; CLI Kit only provides the shared schema and help infrastructure. | ||
|
|
||
| New finite query and operation commands must include `jsonFlag` and expose a `jsonOutputSchema`. The repository lint | ||
| check enforces both. Existing commands are recorded in a temporary migration baseline in | ||
| `packages/eslint-plugin-cli/rules/json-output-legacy-command-paths.js`; remove a command from that baseline when it is | ||
| converted, and never add new commands to it. | ||
|
|
||
| ## Define the result beside the domain service | ||
|
|
||
| Keep the schema beside the service that produces the result. One Zod schema supplies runtime validation, the inferred | ||
|
|
@@ -38,6 +43,11 @@ Expose the contract from the command and encode through it. Encoding validates t | |
|
|
||
| ```ts | ||
| export default class WidgetList extends Command { | ||
| static flags = { | ||
| ...globalFlags, | ||
| ...jsonFlag, | ||
| } | ||
|
|
||
| static get jsonOutputSchema() { | ||
| return widgetListJsonOutputSchema | ||
| } | ||
|
|
@@ -52,6 +62,57 @@ export default class WidgetList extends Command { | |
| } | ||
| ``` | ||
|
|
||
| If the service result and public JSON document differ, keep that mapping in a command-specific codec and validate the | ||
| mapped value with the schema. Presenters continue to own terminal text, output channels, files, and exit behavior. A | ||
| result contract must not depend on terminal rendering, Oclif, filesystem output, or CLI errors. | ||
| ## Keep data and presentation separate | ||
|
|
||
| A finite command should have these boundaries: | ||
|
|
||
| - The domain service returns typed data and doesn't print terminal output. | ||
| - A command-specific codec maps the service result to the stable public JSON shape when they differ. | ||
| - The schema validates and encodes that public result. | ||
| - A presenter turns the same result into human-readable terminal output. | ||
|
|
||
| Presenters continue to own terminal text, output channels, files, and exit behavior. A result contract must not depend | ||
| on terminal rendering, Oclif, filesystem output, or CLI errors. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: Might be worth calling out React/Ink in "terminal rendering" here. |
||
|
|
||
| Events are separate from finite results. Progress events can drive spinners or status messages while the command is | ||
| running, but they aren't fields in the final JSON result. Errors continue through the standard CLI error path and | ||
| stderr; don't encode failures as successful result shapes merely to support `--json`. | ||
|
|
||
| ## Preserve compatibility | ||
|
|
||
| Treat the JSON result as a public API. Keep existing keys, omission rules, nullability, collection shapes, and exit | ||
| behavior when converting a command. Put compatibility mappings in the codec instead of changing domain models or | ||
| leaking presenter details into the schema. Add regression tests for the exact encoded result as well as schema | ||
| validation. | ||
|
|
||
| `--json` selects the output format. `--no-input` controls interactivity. They are independent: JSON output must not | ||
| silently disable prompts, and non-interactive execution must not silently select JSON. A command that can prompt should | ||
| support and test the relevant combinations explicitly. | ||
|
|
||
| ## Exempt only streaming commands | ||
|
|
||
| Long-lived commands that produce an open-ended event stream don't have one finite result. Mark those commands | ||
| explicitly instead of inventing a final JSON document: | ||
|
|
||
| ```ts | ||
| export default class WidgetWatch extends Command { | ||
| static jsonOutputSupport = 'streaming' as const | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do these belong on an "allow list" as well so that we can easily track them? |
||
| } | ||
| ``` | ||
|
|
||
| This exemption is only for commands whose lifetime or output is inherently streaming. A finite operation remains a | ||
| finite command even when it emits progress events, writes a file, or has no interesting return value. | ||
|
|
||
| ## Test a new command | ||
|
|
||
| Tests should verify: | ||
|
|
||
| - the domain service result without terminal concerns; | ||
| - codec compatibility and schema validation; | ||
| - the exact `--json` document; | ||
| - human presentation independently from JSON encoding; | ||
| - errors and exit behavior; and | ||
| - prompt behavior independently from `--json` and `--no-input`. | ||
|
|
||
| Command help includes the generated TypeScript contract automatically through `jsonOutputSchema`. Run the manifest, | ||
| README, and code-documentation refresh commands required by CI after changing command metadata. | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,88 @@ | ||
| const {legacyCommandPaths} = require('./json-output-legacy-command-paths') | ||
|
|
||
| const legacyCommands = new Set(legacyCommandPaths) | ||
|
|
||
| module.exports = { | ||
| meta: { | ||
| type: 'problem', | ||
| docs: { | ||
| description: 'require typed JSON output for new finite commands', | ||
| }, | ||
| schema: [], | ||
| messages: { | ||
| missingJsonFlag: | ||
| 'New finite commands must include ...jsonFlag in their static flags. See docs/cli/json-output.md.', | ||
| missingJsonOutputSchema: | ||
| 'New finite commands must declare a static jsonOutputSchema. See docs/cli/json-output.md.', | ||
| }, | ||
| }, | ||
| create(context) { | ||
| const commandPath = repositoryPath(context.filename) | ||
| if (!isCommandPath(commandPath) || legacyCommands.has(commandPath)) return {} | ||
|
|
||
| return { | ||
| ExportDefaultDeclaration(node) { | ||
| if (node.declaration.type !== 'ClassDeclaration') return | ||
|
|
||
| const classMembers = node.declaration.body.body | ||
| if (hasStreamingExemption(classMembers)) return | ||
|
|
||
| if (!hasJsonOutputSchema(classMembers)) { | ||
| context.report({node: node.declaration, messageId: 'missingJsonOutputSchema'}) | ||
| } | ||
| if (!hasJsonFlag(classMembers)) { | ||
| context.report({node: node.declaration, messageId: 'missingJsonFlag'}) | ||
| } | ||
| }, | ||
| } | ||
| }, | ||
| } | ||
|
|
||
| function isCommandPath(commandPath) { | ||
| return /\/src\/(?:cli\/)?commands\//.test(commandPath) | ||
| } | ||
|
|
||
| function repositoryPath(filename) { | ||
| const normalizedFilename = filename.replaceAll('\\', '/') | ||
| const packagesDirectory = normalizedFilename.lastIndexOf('/packages/') | ||
| return packagesDirectory === -1 ? normalizedFilename : normalizedFilename.slice(packagesDirectory + 1) | ||
| } | ||
|
|
||
| function hasStreamingExemption(classMembers) { | ||
| return classMembers.some( | ||
| (member) => | ||
| isStaticMemberNamed(member, 'jsonOutputSupport') && | ||
| unwrapTypeScriptExpression(member.value)?.value === 'streaming', | ||
| ) | ||
| } | ||
|
|
||
| function hasJsonOutputSchema(classMembers) { | ||
| return classMembers.some( | ||
| (member) => | ||
| member.type === 'MethodDefinition' && member.kind === 'get' && isStaticMemberNamed(member, 'jsonOutputSchema'), | ||
| ) | ||
| } | ||
|
|
||
| function hasJsonFlag(classMembers) { | ||
| const flags = classMembers.find((member) => isStaticMemberNamed(member, 'flags')) | ||
| return flags?.value?.type === 'ObjectExpression' && flags.value.properties.some(isJsonFlagSpread) | ||
| } | ||
|
|
||
| function isStaticMemberNamed(member, name) { | ||
| return member.static && !member.computed && member.key?.name === name | ||
| } | ||
|
|
||
| function isJsonFlagSpread(property) { | ||
| return ( | ||
| property.type === 'SpreadElement' && | ||
| property.argument.type === 'Identifier' && | ||
| property.argument.name === 'jsonFlag' | ||
| ) | ||
| } | ||
|
|
||
| function unwrapTypeScriptExpression(expression) { | ||
| if (expression?.type === 'TSAsExpression' || expression?.type === 'TSSatisfiesExpression') { | ||
| return unwrapTypeScriptExpression(expression.expression) | ||
| } | ||
| return expression | ||
| } |
101 changes: 101 additions & 0 deletions
101
packages/eslint-plugin-cli/rules/command-json-output.test.js
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,101 @@ | ||
| const {RuleTester} = require('eslint') | ||
| const typescriptParser = require('@typescript-eslint/parser') | ||
|
|
||
| const rule = require('./command-json-output') | ||
|
|
||
| const ruleTester = new RuleTester({ | ||
| languageOptions: { | ||
| ecmaVersion: 2022, | ||
| sourceType: 'module', | ||
| parser: typescriptParser, | ||
| }, | ||
| }) | ||
|
|
||
| ruleTester.run('command-json-output', rule, { | ||
| valid: [ | ||
| { | ||
| name: 'finite query command', | ||
| filename: '/repo/packages/app/src/cli/commands/app/widgets/list.ts', | ||
| code: ` | ||
| export default class WidgetList extends Command { | ||
| static flags = {...jsonFlag} | ||
| static get jsonOutputSchema() { | ||
| return widgetListJsonOutputSchema | ||
| } | ||
| } | ||
| `, | ||
| }, | ||
| { | ||
| name: 'finite operation command', | ||
| filename: '/repo/packages/app/src/cli/commands/app/widgets/delete.ts', | ||
| code: ` | ||
| export default class WidgetDelete extends Command { | ||
| static flags = {...globalFlags, ...jsonFlag} | ||
| static get jsonOutputSchema() { | ||
| return widgetDeleteJsonOutputSchema | ||
| } | ||
| } | ||
| `, | ||
| }, | ||
| { | ||
| name: 'streaming command exemption', | ||
| filename: '/repo/packages/app/src/cli/commands/app/widgets/watch.ts', | ||
| code: ` | ||
| export default class WidgetWatch extends Command { | ||
| static jsonOutputSupport = 'streaming' as const | ||
| } | ||
| `, | ||
| }, | ||
| { | ||
| name: 'legacy command baseline', | ||
| filename: '/repo/packages/app/src/cli/commands/app/build.ts', | ||
| code: 'export default class Build extends Command {}', | ||
| }, | ||
| { | ||
| name: 'non-command module', | ||
| filename: '/repo/packages/app/src/cli/services/widgets.ts', | ||
| code: 'export default class WidgetService {}', | ||
| }, | ||
| ], | ||
| invalid: [ | ||
| { | ||
| name: 'new command without JSON support', | ||
| filename: '/repo/packages/app/src/cli/commands/app/widgets/create.ts', | ||
| code: 'export default class WidgetCreate extends Command {}', | ||
| errors: [ | ||
| { | ||
| message: 'New finite commands must declare a static jsonOutputSchema. See docs/cli/json-output.md.', | ||
| }, | ||
| { | ||
| message: 'New finite commands must include ...jsonFlag in their static flags. See docs/cli/json-output.md.', | ||
| }, | ||
| ], | ||
| }, | ||
| { | ||
| name: 'command missing its schema', | ||
| filename: '/repo/packages/app/src/cli/commands/app/widgets/search.ts', | ||
| code: 'export default class WidgetSearch extends Command { static flags = {...jsonFlag} }', | ||
| errors: [ | ||
| { | ||
| message: 'New finite commands must declare a static jsonOutputSchema. See docs/cli/json-output.md.', | ||
| }, | ||
| ], | ||
| }, | ||
| { | ||
| name: 'command missing its JSON flag', | ||
| filename: '/repo/packages/app/src/cli/commands/app/widgets/update.ts', | ||
| code: ` | ||
| export default class WidgetUpdate extends Command { | ||
| static get jsonOutputSchema() { | ||
| return widgetUpdateJsonOutputSchema | ||
| } | ||
| } | ||
| `, | ||
| errors: [ | ||
| { | ||
| message: 'New finite commands must include ...jsonFlag in their static flags. See docs/cli/json-output.md.', | ||
| }, | ||
| ], | ||
| }, | ||
| ], | ||
| }) |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.