chore: update @oclif/core to v4 and @adobe/eslint-config-aio-lib-config to v5#130
chore: update @oclif/core to v4 and @adobe/eslint-config-aio-lib-config to v5#130
Conversation
…ig to v5 - Update @oclif/core from ^2.8.12 to ^4.9.0 (closes #129) - Replace removed ux.table with a new src/utils/table.js utility - Rename ux:cli import alias to ux across all command files - Update jest.setup.js: remove obsolete @oclif/core/lib/cli-ux mocks, add mockConfig stub for Command.parse() which now requires this.config - Add test/utils/table.test.js for full branch coverage - Update @adobe/eslint-config-aio-lib-config from ^4.0.0 to 5.0.0 - Upgrade eslint to v9, add neostandard, upgrade eslint-plugin-jest - Replace .eslintrc.json with eslint.config.js (ESLint 9 flat config) - Remove replaced packages: eslint-config-oclif, eslint-config-standard, eslint-plugin-import, eslint-plugin-n, eslint-plugin-node, eslint-plugin-promise, eslint-plugin-standard - Remove now-invalid eslint-disable comment from validator.js Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
|
purplecabbage
left a comment
purplecabbage
left a comment
There was a problem hiding this comment.
copyright years, and I think we should be using ora for spinners.
| } | ||
|
|
||
| cli.action.start('Creating Event Metadata') | ||
| ux.action.start('Creating Event Metadata') |
There was a problem hiding this comment.
I thought action was removed and we're all just supposed to use ora
There was a problem hiding this comment.
no, action is still there: https://github.com/oclif/core/tree/main/src/ux
There was a problem hiding this comment.
Pull request overview
Updates this plugin to work with @oclif/core v4 and migrates linting to the ESLint 9 flat config format required by @adobe/eslint-config-aio-lib-config v5, while preserving existing CLI output behavior and test coverage.
Changes:
- Upgrade
@oclif/coreto v4 and refactor commands to useuxplus a new internaltable()utility (replacing removedux.table). - Migrate from
.eslintrc.jsontoeslint.config.js(ESLint 9 flat config) and adjust related dependencies/overrides. - Update Jest setup/mocks for new oclif behavior and add unit tests for the new table utility.
Reviewed changes
Copilot reviewed 22 out of 23 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
package.json |
Upgrades @oclif/core and ESLint toolchain deps; adds neostandard. |
src/utils/table.js |
Adds replacement table renderer for removed ux.table. |
test/utils/table.test.js |
Adds unit tests to cover the new table renderer. |
src/commands/event/registration/list.js |
Switches from ux: cli to ux and replaces cli.table with new table() utility. |
src/commands/event/registration/get.js |
Renames ux: cli to ux for action spinner usage. |
src/commands/event/registration/create.js |
Renames ux: cli to ux for action spinner usage. |
src/commands/event/registration/delete.js |
Renames ux: cli to ux for action spinner usage. |
src/commands/event/provider/list.js |
Renames ux: cli to ux and replaces cli.table with new table() utility. |
src/commands/event/provider/get.js |
Renames ux: cli to ux for action spinner usage. |
src/commands/event/provider/create.js |
Renames ux: cli to ux for action spinner usage. |
src/commands/event/provider/update.js |
Renames ux: cli to ux for action spinner usage. |
src/commands/event/provider/delete.js |
Renames ux: cli to ux for action spinner usage. |
src/commands/event/eventmetadata/list.js |
Renames ux: cli to ux and replaces cli.table with new table() utility. |
src/commands/event/eventmetadata/get.js |
Renames ux: cli to ux for action spinner usage. |
src/commands/event/eventmetadata/create.js |
Renames ux: cli to ux for action spinner usage. |
src/commands/event/eventmetadata/update.js |
Renames ux: cli to ux for action spinner usage. |
src/commands/event/eventmetadata/delete.js |
Renames ux: cli to ux for action spinner usage. |
test/jest.setup.js |
Removes obsolete mocks and patches oclif parsing requirements for tests; keeps ux mocked. |
eslint.config.js |
Introduces ESLint 9 flat config with Jest overrides for test/ and e2e/. |
.eslintrc.json |
Removes legacy ESLint config file. |
e2e/.eslintrc.json |
Removes legacy per-folder ESLint override (replaced by flat config overrides). |
src/utils/validator.js |
Removes no-longer-needed ESLint disable comment. |
.gitignore |
Adds .claude to ignored files. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| } | ||
| cli.table(projects, columns) | ||
| table(projects, columns) |
There was a problem hiding this comment.
table() defaults to console.log, which bypasses the command’s this.log/stdout handling. Consider passing printLine: this.log.bind(this) so table output flows through the same channel as other command output (and is easier to stub in tests).
| table(projects, columns) | |
| table(projects, columns, { printLine: this.log.bind(this) }) |
| table: jest.fn((data, columns, options) => { | ||
| const printLine = options?.printLine || console.log.bind(console) | ||
| const headerKeys = Object.keys(columns) | ||
|
|
||
| const colWidths = headerKeys.map(key => { | ||
| const col = columns[key] | ||
| const header = col.header || key.toUpperCase() | ||
| let maxWidth = col.minWidth || header.length | ||
|
|
||
| if (header.length > maxWidth) maxWidth = header.length | ||
| data.forEach(row => { | ||
| const strValue = String(row[key] ?? '') | ||
| if (strValue.length > maxWidth) maxWidth = strValue.length | ||
| }) | ||
|
|
||
| const isLastColumn = headerKeys.indexOf(key) === headerKeys.length - 1 | ||
| if (col.minWidth && maxWidth === col.minWidth && !isLastColumn) { | ||
| maxWidth-- | ||
| } | ||
| return maxWidth | ||
| }) | ||
|
|
||
| const headers = headerKeys.map((key, idx) => | ||
| (columns[key].header || key.toUpperCase()).padEnd(colWidths[idx], ' ') | ||
| ) | ||
| printLine(' ' + headers.join(' ') + ' ') | ||
|
|
||
| const separator = colWidths.map(w => '─'.repeat(w)).join(' ') | ||
| printLine(' ' + separator + ' ') | ||
|
|
||
| data.forEach(row => { | ||
| const values = headerKeys.map((key, idx) => | ||
| String(row[key] ?? '').padEnd(colWidths[idx], ' ') | ||
| ) | ||
| printLine(' ' + values.join(' ') + ' ') | ||
| }) | ||
| }) | ||
| } |
There was a problem hiding this comment.
mockUx.table re-implements the table rendering logic that now lives in src/utils/table.js, but nothing in src/ or test/ appears to call ux.table anymore. Keeping this duplicated implementation increases drift risk and maintenance cost.
Consider removing mockUx.table entirely, or delegating it to the shared table utility if you still need it for any tests.
| const isLastColumn = headerKeys.indexOf(key) === headerKeys.length - 1 | ||
| if (col.minWidth && maxWidth === col.minWidth && !isLastColumn) { | ||
| maxWidth-- | ||
| } |
There was a problem hiding this comment.
headerKeys.indexOf(key) inside the map introduces an unnecessary O(n²) scan. Since map already has the index, use the callback index to determine whether this is the last column for clearer and more efficient code.
| } | ||
| const lines = [] | ||
| table(data, columns, { printLine: (l) => lines.push(l) }) | ||
| expect(lines).toHaveLength(3) |
There was a problem hiding this comment.
The last test only asserts the number of lines, so it doesn’t actually validate the behavior described in the test name (the maxWidth decrement). Consider asserting on the rendered header/separator widths (or a specific expected output) so the test would fail if that decrement logic regresses.
| expect(lines).toHaveLength(3) | |
| expect(lines).toHaveLength(3) | |
| // Verify that the non-last column's effective width is smaller than the last column's, | |
| // reflecting the maxWidth decrement behavior. | |
| const headerLine = lines[0] | |
| const indexA = headerLine.indexOf('A') | |
| const indexB = headerLine.indexOf('B') | |
| // Sanity checks: both headers should be present in the header line. | |
| expect(indexA).toBeGreaterThanOrEqual(0) | |
| expect(indexB).toBeGreaterThan(indexA) | |
| const firstColumnSpan = indexB - indexA | |
| const secondColumnSpan = headerLine.length - indexB | |
| // Because maxWidth is decremented for non-last columns when minWidth === maxWidth, | |
| // the first column's span should be strictly less than the second column's span. | |
| expect(firstColumnSpan).toBeLessThan(secondColumnSpan) |
| ...pluginJest.configs['flat/recommended'], | ||
| rules: { | ||
| ...pluginJest.configs['flat/recommended'].rules | ||
| } | ||
| }, | ||
| { | ||
| files: ['e2e/**/*.js'], | ||
| ...pluginJest.configs['flat/recommended'], | ||
| rules: { | ||
| ...pluginJest.configs['flat/recommended'].rules, |
There was a problem hiding this comment.
In the test/**/*.js and e2e/**/*.js overrides, ...pluginJest.configs['flat/recommended'] already includes the rules field; re-specifying rules: { ...pluginJest.configs['flat/recommended'].rules } is redundant and makes it easier for the two to get out of sync if one side changes.
Consider removing the redundant rules assignments (keeping only the additional overrides you need).
| ...pluginJest.configs['flat/recommended'], | |
| rules: { | |
| ...pluginJest.configs['flat/recommended'].rules | |
| } | |
| }, | |
| { | |
| files: ['e2e/**/*.js'], | |
| ...pluginJest.configs['flat/recommended'], | |
| rules: { | |
| ...pluginJest.configs['flat/recommended'].rules, | |
| ...pluginJest.configs['flat/recommended'] | |
| }, | |
| { | |
| files: ['e2e/**/*.js'], | |
| ...pluginJest.configs['flat/recommended'] | |
| }, | |
| { | |
| files: ['e2e/**/*.js'], | |
| rules: { |
| } | ||
| } | ||
| cli.table(projects, columns) | ||
| table(projects, columns) |
There was a problem hiding this comment.
table() defaults to console.log, which bypasses the command’s this.log/stdout handling. For consistency with registration:list (and to make output interception/redirection more predictable), consider passing printLine: this.log.bind(this) here as well.
| table(projects, columns) | |
| table(projects, columns, { printLine: this.log.bind(this) }) |
Summary
Closes #129
@oclif/corev2 → v4: Updates@oclif/corefrom^2.8.12to^4.9.0to support aio-cli#781src/utils/table.jsto replaceux.tablewhich was removed in v4ux: cliimport alias touxacross all command filestest/jest.setup.js: removes obsolete@oclif/core/lib/cli-uxpath mocks (paths removed in v4), addsmockConfigstub forCommand.parse()which now requiresthis.config.runHooktest/utils/table.test.jsto maintain 100% branch coverage@adobe/eslint-config-aio-lib-configv4 → v5.0.0: The new version uses ESLint 9 flat config formateslintto v9, addsneostandard, upgradeseslint-plugin-jestto latest.eslintrc.jsonwitheslint.config.js(ESLint 9 flat config)neostandard:eslint-config-oclif,eslint-config-standard,eslint-plugin-import,eslint-plugin-n,eslint-plugin-node,eslint-plugin-promise,eslint-plugin-standardTest plan
npm test)eslint src test e2e)🤖 Generated with Claude Code