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'
})
}
| description: 'The action to call' | ||
| } | ||
| ] | ||
| ApiCreate.args = { |
There was a problem hiding this comment.
nit: use newlines in formatting so this can be read easier
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.
| readdirSync: jest.fn((path) => { | ||
| const files = [] | ||
| for (const filePath in global.__mockFs) { | ||
| if (filePath.startsWith(path)) { | ||
| files.push(filePath.replace(path + '/', '')) | ||
| } | ||
| } | ||
| return files.length > 0 ? files : actualFs.readdirSync(path) |
There was a problem hiding this comment.
The custom mock implementation for readdirSync doesn't handle paths without trailing slashes correctly. When a path doesn't end with '/', the replace operation filePath.replace(path + '/', '') may not work as expected. Consider normalizing paths or handling edge cases where filePath equals path exactly.
| 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.
|
|
||
| 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 |
Description
Related Issue
Motivation and Context
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist: