fix(x-skill): load ora with dynamic import - #1980
Conversation
dosubot
Bot
added
bug
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough将 Changesora CJS 兼容性
Estimated code review effort: 2 (Simple) | ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the x-skill package to dynamically import the ESM-only ora library, ensuring compatibility with CommonJS builds. The feedback suggests optimizing the dynamic import helper by caching the module and simplifying the loader function. Additionally, the reviewer points out a bug where the spinner is not stopped if an error occurs during version listing, and provides a robust refactoring suggestion to handle errors gracefully.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| type Ora = typeof import('ora').default; | ||
|
|
||
| async function loadOra(): Promise<Ora> { | ||
| // Keep native dynamic import in the CJS build because ora is ESM-only. | ||
| const loader = new Function('specifier', 'return import(specifier)') as ( | ||
| specifier: string, | ||
| ) => Promise<typeof import('ora')>; | ||
|
|
||
| const { default: ora } = await loader('ora'); | ||
| return ora; | ||
| } |
There was a problem hiding this comment.
The dynamic import helper can be simplified by removing the unused specifier parameter from the dynamic function constructor. Additionally, caching the loaded ora module prevents redundant dynamic imports and function recreations if loadOra is called multiple times in the future. Renaming the type to OraFactory also avoids potential confusion with the Ora spinner instance type.
type OraFactory = typeof import('ora').default;
let oraCache: OraFactory | null = null;
async function loadOra(): Promise<OraFactory> {
if (oraCache) {
return oraCache;
}
// Keep native dynamic import in the CJS build because ora is ESM-only.
const loader = new Function('return import("ora")') as () => Promise<{ default: OraFactory }>;
const { default: ora } = await loader();
oraCache = ora;
return ora;
}|
|
||
| async listVersions(): Promise<void> { | ||
| try { | ||
| const ora = await loadOra(); |
There was a problem hiding this comment.
If this.skillLoader.listVersions() throws an error, spinner.stop() is never called because the execution immediately jumps to the catch block. This can leave the spinner running indefinitely or result in garbled console output when the error message is printed.
Consider refactoring the method to ensure the spinner is stopped in case of an error:
async listVersions(): Promise<void> {
let spinner: any;
try {
const ora = await loadOra();
spinner = ora(
this.helpManager.colorize(getMessage('fetchingVersions', this.language), 'cyan'),
).start();
const versions = await this.skillLoader.listVersions();
spinner.stop();
if (versions.length === 0) {
console.log(
`${this.helpManager.colorize(getMessage('noVersionsFound', this.language), 'yellow')}`,
);
return;
}
console.log(
`${this.helpManager.colorize(getMessage('availableVersions', this.language), 'green')}`,
);
versions.forEach((version, index) => {
const marker = index === 0 ? getMessage('latestMarker', this.language) : '';
console.log(` ${this.helpManager.colorize(version, 'yellow')}${marker}`);
});
} catch (error) {
if (spinner) {
spinner.stop();
}
console.error(
`${this.helpManager.colorize(getMessage('error', this.language), 'red')} ${(error as Error).message}`,
);
}
}![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/x-skill/src/index.ts (1)
205-210: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win确保在发生错误时停止 spinner。
如果
this.skillLoader.listVersions()抛出异常，代码会直接跳入catch块，导致spinner.stop()被跳过。虽然当前逻辑下遇到错误进程会退出，但在退出前，未停止的 spinner 可能会干扰或覆盖终端中的错误信息输出。建议将 spinner 的引用提升，并在发生异常时确保将其停止。💡 修复建议
async listVersions(): Promise<void> { + let spinner: ReturnType<Ora> | undefined; try { const ora = await loadOra(); - const spinner = ora( + spinner = ora( this.helpManager.colorize(getMessage('fetchingVersions', this.language), 'cyan'), ).start(); const versions = await this.skillLoader.listVersions(); spinner.stop();然后在下方的
catch块中调用停止方法：} catch (error) { + spinner?.stop(); console.error( `${this.helpManager.colorize(getMessage('error', this.language), 'red')} ${(error as Error).message}`, ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/x-skill/src/index.ts` around lines 205 - 210, 在包含 `loadOra`、spinner 创建和 `this.skillLoader.listVersions()` 的流程中，将 spinner 引用提升到 try 块外，并在 catch 块中确保已创建的 spinner 被停止；保留成功路径现有的停止逻辑，避免异常时终端输出被活动 spinner 干扰。
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/x-skill/src/index.ts`:
- Around line 205-210: 在包含 `loadOra`、spinner 创建和
`this.skillLoader.listVersions()` 的流程中，将 spinner 引用提升到 try 块外，并在 catch 块中确保已创建的
spinner 被停止；保留成功路径现有的停止逻辑，避免异常时终端输出被活动 spinner 干扰。
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 8b8048f1-ba96-47a1-aa71-e8a104338366
📒 Files selected for processing (1)
packages/x-skill/src/index.ts
2 participants
🤔 This is a ...
🔗 Related Issues
Fixes #1971
💡 Background and Solution
@ant-design/x-skillbuilds a CommonJS CLI entry, butora@9is ESM-only. The current top-level static import is transformed intorequire('ora')in the CJS output, causingERR_REQUIRE_ESMwhen running the CLI.This PR removes the top-level
oraimport and loads it only when needed through native dynamic import, so the CJS CLI can start normally and the spinner path can still useora.Verified with:
npm run tsc --workspace packages/x-skillnpm run lint --workspace packages/x-skillnpm run compile --workspace packages/x-skillnode packages\x-skill\bin\index.js --helpnode packages\x-skill\bin\index.js --list-versions📝 Change Log
Summary by CodeRabbit