Update @adobe/aio-cli-plugin-runtime to @oclif/core v2 #390
Conversation
purplecabbage
left a comment
purplecabbage
left a comment
There was a problem hiding this comment.
Some nits on how json arg objects are formatted, we prefer readability. ex.
RuleCreate.args = {
name: Args.string({
required: true,
description: 'Name of the rule'
}),
trigger: Args.string({
required: true,
description: 'Name of the trigger'
}),
action: Args.string({
required: true,
description: 'Name of the action'
})
}
There was a problem hiding this comment.
Pull request overview
This pull request migrates the @adobe/aio-cli-plugin-runtime from @oclif/core v1 to v2. The migration involves significant changes to the CLI framework API.
Changes:
- Updated @oclif/core dependency from v1.3.0 to v2.8.12
- Converted args definitions from array-based to object-based syntax using Args.string()
- Replaced deprecated CliUx.cli references with ux
- Replaced jest-plugin-fs with a custom filesystem mock implementation
- Updated test assertions to work with new args structure
- Removed leading slashes from test file paths to match the custom mock filesystem
- Updated error message assertions to match @oclif/core v2 error format
Reviewed changes
Copilot reviewed 86 out of 86 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| package.json | Updated @oclif/core to v2.8.12, removed jest-plugin-fs dependency |
| test/jest.setup.js | Implemented custom filesystem mock to replace jest-plugin-fs |
| src/commands/runtime/**/*.js | Converted args from array to object syntax, updated imports for Args and ux |
| test/commands/**/*.test.js | Updated test assertions for object-based args, changed test file paths, updated error message expectations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| mkdirSync: jest.fn((path, options) => { | ||
| if (!global.__mockFs[path]) { | ||
| global.__mockFs[path] = null | ||
| } | ||
| if (options && options.recursive) { | ||
| const parts = path.split('/').filter(p => p) | ||
| let currentPath = '' | ||
| for (const part of parts) { | ||
| currentPath = currentPath ? `${currentPath}/${part}` : part | ||
| if (!global.__mockFs.hasOwnProperty(currentPath)) { | ||
| global.__mockFs[currentPath] = null | ||
| } | ||
| } | ||
| } | ||
| }) |
There was a problem hiding this comment.
The mkdirSync mock implementation stores null for directory entries but doesn't differentiate between directories and files when checking with statSync. The statSync function always returns isDirectory: false, even for directory paths. This could cause issues if code checks whether a path is a directory.
| statSync: jest.fn((path) => { | ||
| if (global.__mockFs.hasOwnProperty(path)) { | ||
| return { | ||
| isFile: () => true, | ||
| isDirectory: () => false, | ||
| size: typeof global.__mockFs[path] === 'string' ? Buffer.byteLength(global.__mockFs[path]) : global.__mockFs[path].length | ||
| } | ||
| } | ||
| return actualFs.statSync(path) | ||
| }), |
There was a problem hiding this comment.
The statSync mock implementation may fail when global.__mockFs[path] is null (as set by mkdirSync for directories). Attempting to access .length property on null will cause a runtime error. Consider handling the case where content is null or undefined.
src/commands/runtime/api/get.js
Outdated
|
|
||
| const RuntimeBaseCommand = require('../../../RuntimeBaseCommand') | ||
| const { Args } = require('@oclif/core') | ||
| // eslint-disable-next-line no-unused-vars |
There was a problem hiding this comment.
The eslint-disable comment for no-unused-vars appears to be unnecessary as all imports are now being used. The Args import is actively used in the args definition. Consider removing this unused eslint-disable comment.
| // eslint-disable-next-line no-unused-vars |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
|
||
| test('args', async () => { | ||
| expect(TheCommand.args).not.toBeDefined() | ||
| /* eslint-disable jest/no-conditional-expect */ |
There was a problem hiding this comment.
let's not do this disable. write two tests.
|
|
||
| test('args', async () => { | ||
| expect(TheCommand.args).toBeUndefined() | ||
| /* eslint-disable jest/no-conditional-expect */ |
There was a problem hiding this comment.
let's not do this disable. write two tests.
|
|
||
| test('args', async () => { | ||
| expect(TheCommand.args).not.toBeDefined() | ||
| /* eslint-disable jest/no-conditional-expect */ |
There was a problem hiding this comment.
let's not do this disable. write two tests.
|
|
||
| test('args', async () => { | ||
| expect(TheCommand.args).toBeUndefined() | ||
| /* eslint-disable jest/no-conditional-expect */ |
There was a problem hiding this comment.
let's not do this disable. write two tests.
|
|
||
| test('args', async () => { | ||
| expect(TheCommand.args).not.toBeDefined() | ||
| /* eslint-disable jest/no-conditional-expect */ |
There was a problem hiding this comment.
let's not do this disable. write two tests.
|
|
||
| test('args', async () => { | ||
| expect(TheCommand.args).toBeUndefined() | ||
| /* eslint-disable jest/no-conditional-expect */ |
There was a problem hiding this comment.
let's not do this disable. write two tests.
|
|
||
| test('args', async () => { | ||
| expect(TheCommand.args).toBeUndefined() | ||
| /* eslint-disable jest/no-conditional-expect */ |
There was a problem hiding this comment.
let's not do this disable. write two tests.
|
|
||
| test('args', async () => { | ||
| expect(TheCommand.args).not.toBeDefined() | ||
| /* eslint-disable jest/no-conditional-expect */ |
There was a problem hiding this comment.
let's not do this disable. write two tests.
test/commands/runtime/index.test.js
Outdated
|
|
||
| test('args', async () => { | ||
| expect(TheCommand.args).toBeUndefined() | ||
| /* eslint-disable jest/no-conditional-expect */ |
There was a problem hiding this comment.
let's not do this disable. write two tests.
test/RuntimeBaseCommand.test.js
Outdated
|
|
||
| test('args', async () => { | ||
| expect(TheCommand.args).toBeUndefined() | ||
| /* eslint-disable jest/no-conditional-expect */ |
There was a problem hiding this comment.
let's not do this disable. write two tests.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 86 out of 86 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/commands/runtime/api/list.js
Outdated
| // Workaround for oclif v2 parsing issue: capture argv before parse() when multiple optional args are present | ||
| // oclif v2 doesn't properly parse --json flag when command has 3+ optional positional arguments | ||
| // Related: https://github.com/oclif/core/issues/854 (workaround: search argv directly) | ||
| const argvBeforeParse = this.argv ? [...this.argv] : [] |
There was a problem hiding this comment.
The workaround for handling argv checks if this.argv is truthy before spreading it, but doesn't account for this.argv being an empty array. When argv is undefined, the ternary operator correctly returns an empty array, but the spread operator will still fail if this.argv is null. Consider using const argvBeforeParse = this.argv ?? [] which is more robust and handles null/undefined consistently.
| const argvBeforeParse = this.argv ? [...this.argv] : [] | |
| const argvBeforeParse = this.argv ?? [] |
Description
Upgraded the @adobe/aio-cli-plugin-runtime to @oclif/core v2 by updating the dependency to ^2.8.12, switching from CliUx to direct ux imports and refactored command arguments to the new object-based syntax in line with the v2 migration guide.
Motivation and Context
https://jira.corp.adobe.com/browse/ACNA-2491
How Has This Been Tested?
Yes, npm test
Screenshots (if appropriate):
Types of changes
Checklist: