-
Notifications
You must be signed in to change notification settings - Fork 7
chore: adjust merge logic #175
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
Conversation
概览修改了Jest配置文件的加载机制。替换了单一的配置查询方式,改为通用的merge流程,支持检测多种配置文件格式(jest.config.js/ts/json和.jest.config.js/ts/json),加载首个存在的配置文件,支持默认导出和命名导出,并合并restConfig与现有配置,保留并组合setupFiles和setupFilesAfterEnv。 变更
预计代码审查工作量🎯 2 (简单) | ⏱️ ~10 分钟
诗歌
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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 |
Summary of ChangesHello @zombieJ, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! 此拉取请求旨在增强 Jest 配置文件的加载和合并逻辑。通过扩展对多种配置文件格式的支持,并改进查找机制,它提高了系统与用户自定义配置的兼容性和可靠性,确保配置能够正确地被识别和应用。 Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/index.ts (1)
86-97: 建议添加错误处理以提高健壮性。第 87 行的
require()调用缺少错误处理。如果用户的配置文件存在语法错误、模块解析失败或其他问题,程序将直接崩溃,而不是给出有用的错误信息。建议添加 try-catch 块来优雅地处理加载失败的情况:
🔎 建议的错误处理实现
if (userJestConfigFile) { - let userConfig = require(userJestConfigFile); - userConfig = userConfig.default || userConfig; - - const { setupFiles = [], setupFilesAfterEnv = [], ...restConfig } = userConfig; - config = { - ...config, - ...restConfig, - setupFiles: [...config.setupFiles, ...setupFiles], - setupFilesAfterEnv: [...config.setupFilesAfterEnv, ...setupFilesAfterEnv], - }; + try { + let userConfig = require(userJestConfigFile); + userConfig = userConfig.default || userConfig; + + const { setupFiles = [], setupFilesAfterEnv = [], ...restConfig } = userConfig; + config = { + ...config, + ...restConfig, + setupFiles: [...config.setupFiles, ...setupFiles], + setupFilesAfterEnv: [...config.setupFilesAfterEnv, ...setupFilesAfterEnv], + }; + } catch (error) { + console.error(`[rc-test] Failed to load Jest config from ${userJestConfigFile}:`, error.message); + // 可选:继续使用默认配置或抛出错误 + } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/index.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/index.ts (1)
bin/rc-test-migrate.js (3)
path(3-3)cwd(12-12)fs(5-5)
🔇 Additional comments (1)
src/index.ts (1)
90-96: 合并逻辑正确。配置合并逻辑实现得当:
- 使用解构和扩展运算符将
restConfig覆盖基础配置setupFiles和setupFilesAfterEnv数组正确地保留了顺序(基础配置在前,用户配置在后)- 这确保了 rc-test 的内置设置文件在用户自定义设置之前执行
| // Merge Jest config file (support .js, .ts, .json) | ||
| let userJestConfigFile = null; | ||
| const possibleConfigFiles = [ | ||
| 'jest.config.js', | ||
| 'jest.config.ts', | ||
| 'jest.config.json', | ||
| '.jest.config.js', | ||
| '.jest.config.ts', | ||
| '.jest.config.json', | ||
| ]; | ||
|
|
||
| for (let i = 0; i < possibleConfigFiles.length; i++) { | ||
| const configPath = path.resolve(cwd, possibleConfigFiles[i]); | ||
| if (fs.existsSync(configPath)) { | ||
| userJestConfigFile = configPath; | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
# 检查 package.json 中的依赖和脚本
cat package.json | head -50Repository: react-component/rc-test
Length of output: 1386
🏁 Script executed:
# 搜索是否存在 TypeScript 加载器相关配置
rg "ts-node|@swc-node/register|tsx|esbuild-register|register" -t json -t js -t ts --max-count 20Repository: react-component/rc-test
Length of output: 141
🏁 Script executed:
# 查看 src/index.ts 完整实现(第 67-97 行)
sed -n '60,100p' src/index.tsRepository: react-component/rc-test
Length of output: 1174
🏁 Script executed:
# 检查是否存在 jest.config.ts 或 .jest.config.ts 文件
fd -t f "^\.?jest\.config\.(ts|js|json)$" --max-depth 2Repository: react-component/rc-test
Length of output: 81
🏁 Script executed:
# 获取完整的 package.json
cat package.jsonRepository: react-component/rc-test
Length of output: 2062
🏁 Script executed:
# 检查 bin/rc-test.js 是否有 TypeScript 加载器注册
cat bin/rc-test.jsRepository: react-component/rc-test
Length of output: 438
🏁 Script executed:
# 检查是否有 tsconfig.json
cat tsconfig.json 2>/dev/null || echo "no tsconfig.json found"Repository: react-component/rc-test
Length of output: 89
🏁 Script executed:
# 检查编译后的 lib/index.js 文件是否存在及其内容
head -100 lib/index.js | grep -A 20 "possibleConfigFiles"Repository: react-component/rc-test
Length of output: 138
🏁 Script executed:
# 验证是否有任何 TypeScript 加载器在 node_modules 或项目中
rg "ts-node|@swc-node|tsx|esbuild-register" src/ bin/ --type ts --type jsRepository: react-component/rc-test
Length of output: 141
处理 TypeScript 配置文件的加载支持。
代码在第 70 和 74 行列出了 jest.config.ts 和 .jest.config.ts 作为可能的配置文件,但第 87 行使用 require() 加载配置文件。项目的依赖中不包含 ts-node、@swc-node/register、tsx 或其他 TypeScript 加载器,因此当用户提供 TypeScript 配置文件时,require() 会失败。
建议:
- 移除 .ts 配置文件选项,或
- 在运行时添加 TypeScript 加载器支持,或
- 为
.ts文件添加错误处理并改用.js配置
🤖 Prompt for AI Agents
In src/index.ts around lines 67 to 84, the code lists jest.config.ts and
.jest.config.ts as possible config files but later uses require() to load the
chosen file, which will fail for TypeScript files without a runtime loader;
remove the .ts entries from possibleConfigFiles (only keep .js and .json
variants) or add detection for .ts files and handle them explicitly by either
(a) attempting to resolve a same-named .js/.json fallback and using that, or (b)
loading a TypeScript runtime (e.g., require('ts-node/register') or similar)
before require() — implement the simpler approach of removing .ts from the list
and, if you prefer to keep .ts support, add a clear error message that instructs
the user to install a TypeScript loader when a .ts config is found.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
本次 PR 改进了 Jest 配置文件的合并逻辑,增加了对多种配置文件格式(.js, .ts, .json)的支持,并修复了用户配置中 setupFiles 和 setupFilesAfterEnv 未被正确合并的 bug。这些都是很好的增强。
我提供了一个关于配置文件查找逻辑的建议,建议使用 Array.prototype.map 和 Array.prototype.find 来替代当前的 for 循环,这样可以让代码更简洁、更具可读性。
| let userJestConfigFile = null; | ||
| const possibleConfigFiles = [ | ||
| 'jest.config.js', | ||
| 'jest.config.ts', | ||
| 'jest.config.json', | ||
| '.jest.config.js', | ||
| '.jest.config.ts', | ||
| '.jest.config.json', | ||
| ]; | ||
|
|
||
| for (let i = 0; i < possibleConfigFiles.length; i++) { | ||
| const configPath = path.resolve(cwd, possibleConfigFiles[i]); | ||
| if (fs.existsSync(configPath)) { | ||
| userJestConfigFile = configPath; | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Summary by CodeRabbit
发布说明
✏️ Tip: You can customize this high-level summary in your review settings.