-
Notifications
You must be signed in to change notification settings - Fork 9
fix: 解决node23版本不兼容+webpack持久缓存修复 #115
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
📝 WalkthroughWalkthroughAdds a creation-time install event handler to inject package-manager-specific overrides mapping Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as Developer
participant CLI as mpx-cli (create)
participant Events as Installer Events
participant FS as targetDir/package.json
participant PM as Package Manager
Dev->>CLI: start project creation
CLI->>CLI: resolve packageManager (options, loadOptions, hasYarn, hasPnpm3OrLater)
CLI->>Events: register install-event handler
Events->>CLI: install event fires (plugins-install / deps-install)
CLI->>FS: read package.json
CLI->>CLI: inject override `@achrinza/node-ipc` → `npm:node-ipc-compat@1.0.0` (pnpm or npm format)
CLI->>FS: write updated package.json (try/catch)
FS->>PM: package.json used during install
PM->>FS: install respects override
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧬 Code graph analysis (1)packages/vue-cli-plugin-mpx/commands/serve/web.js (1)
🔇 Additional comments (1)
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 |
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: 2
🧹 Nitpick comments (1)
packages/mpx-cli/lib/create.js (1)
207-207: Consider logging errors instead of silently swallowing them.The empty catch block silently ignores any errors during package.json manipulation. While this prevents the creation flow from failing, it leaves users unaware if the Node 23 compatibility fix wasn't applied.
🔎 Proposed improvement
- } catch (e) {} + } catch (e) { + // Non-fatal: log but don't fail project creation + console.warn('Warning: Failed to inject @achrinza/node-ipc override:', e.message) + }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
README.md(1 hunks)packages/mpx-cli/lib/create.js(2 hunks)packages/vue-cli-plugin-mpx/config/base.js(1 hunks)
🔇 Additional comments (4)
packages/mpx-cli/lib/create.js (2)
10-13: LGTM!The additional imports for package manager detection are appropriate for the new functionality.
182-186: LGTM!The package manager detection logic correctly follows the priority chain: explicit option → saved option → yarn → pnpm → npm fallback.
README.md (1)
82-93: LGTM!The documentation clearly explains the Node 23 compatibility issue and the automatic workaround. The JSON examples for both pnpm and npm match the implementation in create.js.
packages/vue-cli-plugin-mpx/config/base.js (1)
475-475: The implementation properly handles the change fromvue.config.jstompx.config.js. The generator createsmpx.config.jsin new projects and removesvue.config.js, while webpack cache invalidation is correctly configured usingmpx.config.jsin the buildDependencies array at line 475. No further verification is needed.Likely an incorrect or invalid review comment.
| creator.on('creation', ({ event }) => { | ||
| if (event === 'plugins-install' || event === 'deps-install') { | ||
| try { | ||
| const pkgPath = path.resolve(targetDir, 'package.json') | ||
| const pkg = fs.readJsonSync(pkgPath) | ||
| // 根据包管理器类型写入对应的 overrides 配置,yarn无需处理 | ||
| if (packageManager === 'pnpm') { | ||
| pkg.pnpm = { | ||
| overrides: { | ||
| '@achrinza/node-ipc': 'npm:node-ipc-compat@1.0.0' | ||
| } | ||
| } | ||
| } else if (packageManager === 'npm') { | ||
| pkg.overrides = { | ||
| '@achrinza/node-ipc': 'npm:node-ipc-compat@1.0.0' | ||
| } | ||
| } | ||
| fs.writeJsonSync(pkgPath, pkg, { spaces: 2 }) | ||
| } catch (e) {} | ||
| } | ||
| }) |
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.
Risk of overwriting existing pnpm/npm configurations.
The code completely replaces pkg.pnpm and pkg.overrides objects, which could delete any existing configurations users may have. This is especially problematic if other CLI plugins or user customizations have already added pnpm or npm overrides.
🔎 Proposed fix to merge instead of replace
// 根据包管理器类型写入对应的 overrides 配置,yarn无需处理
if (packageManager === 'pnpm') {
- pkg.pnpm = {
- overrides: {
- '@achrinza/node-ipc': 'npm:node-ipc-compat@1.0.0'
- }
- }
+ pkg.pnpm = pkg.pnpm || {}
+ pkg.pnpm.overrides = pkg.pnpm.overrides || {}
+ pkg.pnpm.overrides['@achrinza/node-ipc'] = 'npm:node-ipc-compat@1.0.0'
} else if (packageManager === 'npm') {
- pkg.overrides = {
- '@achrinza/node-ipc': 'npm:node-ipc-compat@1.0.0'
- }
+ pkg.overrides = pkg.overrides || {}
+ pkg.overrides['@achrinza/node-ipc'] = 'npm:node-ipc-compat@1.0.0'
}🤖 Prompt for AI Agents
In packages/mpx-cli/lib/create.js around lines 189 to 209, the code currently
replaces pkg.pnpm and pkg.overrides wholesale which can wipe out existing user
or plugin configurations; instead, read package.json (already done), ensure the
pnpm and overrides objects exist, and merge the new entry into the existing
objects (perform a shallow merge for pkg.pnpm and pkg.overrides, and
specifically merge into pkg.pnpm.overrides if using pnpm) rather than assigning
a new object; create the objects if missing, preserve existing keys, then write
the merged pkg back; also keep the try/catch but consider logging the error
instead of swallowing it.
| if (packageManager === 'pnpm') { | ||
| pkg.pnpm = { | ||
| overrides: { | ||
| '@achrinza/node-ipc': 'npm:node-ipc-compat@1.0.0' |
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:
grep -r "node-ipc-compat" --include="*.js" --include="*.json" .Repository: mpx-ecology/mpx-cli
Length of output: 351
The package node-ipc-compat@1.0.0 does not exist on the NPM registry and will cause installation to fail.
The hardcoded alias to npm:node-ipc-compat@1.0.0 cannot be resolved. Web searches show no published package by this name or version. Similar alternatives like @node-ipc/compat (version 9.2.5) exist but not the referenced package. This dependency must be corrected before the code can be used in production, as any package installation will fail with a resolution error.
🤖 Prompt for AI Agents
In packages/mpx-cli/lib/create.js around line 198 the dependency alias
'@achrinza/node-ipc': 'npm:node-ipc-compat@1.0.0' points to a non-existent
package and will break installs; replace it with a valid package/version (for
example use the published @node-ipc/compat with a concrete version like 9.2.5 or
remove the alias and reference the correct package name directly), update the
lock/manifest if present, and verify by running an install to ensure the package
resolves from the NPM registry.
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
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/vue-cli-plugin-mpx-ssr/commands/build/index.jspackages/vue-cli-plugin-mpx/commands/build/index.jspackages/vue-cli-plugin-mpx/commands/serve/mp.jspackages/vue-cli-plugin-mpx/commands/serve/web.jspackages/vue-cli-plugin-mpx/utils/output.jspackages/vue-cli-plugin-mpx/utils/webpack.js
🧰 Additional context used
🧬 Code graph analysis (4)
packages/vue-cli-plugin-mpx/commands/build/index.js (4)
packages/vue-cli-plugin-mpx/commands/inspect.js (1)
args(19-19)packages/mpx-cli-service/bin/mpx-cli-service.js (1)
args(29-44)scripts/test.js (1)
args(3-3)packages/vue-cli-plugin-mpx/index.js (1)
options(9-9)
packages/vue-cli-plugin-mpx/commands/serve/mp.js (1)
packages/vue-cli-plugin-mpx/index.js (1)
options(9-9)
packages/vue-cli-plugin-mpx/utils/webpack.js (1)
packages/vue-cli-plugin-mpx/index.js (1)
options(9-9)
packages/vue-cli-plugin-mpx-ssr/commands/build/index.js (3)
packages/vue-cli-plugin-mpx-ssr/commands/serve/serveServer.js (6)
require(1-1)require(2-2)require(3-3)require(4-4)require(8-8)target(13-13)packages/vue-cli-plugin-mpx/commands/build/index.js (1)
target(39-39)packages/vue-cli-plugin-mpx/commands/serve/mp.js (1)
target(9-9)
🔇 Additional comments (7)
packages/vue-cli-plugin-mpx/utils/output.js (1)
70-70: LGTM! Colors disabled in CI environments.The conditional color output (
colors: !process.env.CI) correctly disables colors in CI environments while keeping them enabled locally, which is a common best practice for CI/CD pipelines.packages/vue-cli-plugin-mpx/commands/build/index.js (1)
55-55: LGTM! Consistent with the updated signature.The addition of the
optionsparameter correctly aligns with the extendedhandleWebpackDonesignature and enables custom stats configuration.packages/vue-cli-plugin-mpx/utils/webpack.js (2)
4-5: LGTM! Well-designed extension with backward compatibility.The function signature extension with a default parameter value (
options = {}) ensures backward compatibility, and the optional chaining safely extracts stats options from nested properties.
23-23: LGTM! Stats options properly propagated.The
statsOptionsare correctly passed toextractResultFromStats, enabling custom stats rendering configuration.packages/vue-cli-plugin-mpx/commands/serve/web.js (1)
275-278: LGTM! Stats customization properly merged.The stats configuration correctly merges the default
assets: falsewith plugin-provided options, allowing customization while preserving existing defaults.packages/vue-cli-plugin-mpx/commands/serve/mp.js (1)
17-17: LGTM! Consistent with the updated signature.The addition of the
optionsparameter aligns with the extendedhandleWebpackDonesignature and enables stats customization in serve mode.packages/vue-cli-plugin-mpx-ssr/commands/build/index.js (1)
1-1: LGTM! Import consolidation.Good refactoring to include
getCurrentTargetin the destructured imports, removing the need for a separate require statement.
| // todo:此处target应该是一个布尔值,判断是否watch, 直接把target和api替换掉有问题嘛,当前只将api换为了options | ||
| handleWebpackDone(err, stats, target, options) |
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.
Critical: Incorrect parameter type passed to handleWebpackDone.
The third parameter of handleWebpackDone(err, stats, watch, options) expects a boolean indicating watch mode, but target (a platform identifier string like 'wx', 'ali', etc.) is being passed instead. This will cause incorrect behavior in the webpack completion handler.
For build commands, the watch parameter should typically be:
false(for standard builds), orargs.watch(if watch mode is supported via CLI args)
🔎 Proposed fix
- // todo:此处target应该是一个布尔值,判断是否watch, 直接把target和api替换掉有问题嘛,当前只将api换为了options
- handleWebpackDone(err, stats, target, options)
+ handleWebpackDone(err, stats, false, options)Or if watch mode should be supported:
- // todo:此处target应该是一个布尔值,判断是否watch, 直接把target和api替换掉有问题嘛,当前只将api换为了options
- handleWebpackDone(err, stats, target, options)
+ handleWebpackDone(err, stats, args.watch || false, options)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // todo:此处target应该是一个布尔值,判断是否watch, 直接把target和api替换掉有问题嘛,当前只将api换为了options | |
| handleWebpackDone(err, stats, target, options) | |
| handleWebpackDone(err, stats, false, options) |
🤖 Prompt for AI Agents
In packages/vue-cli-plugin-mpx-ssr/commands/build/index.js around lines 44-45,
the third argument passed to handleWebpackDone is currently `target` (a platform
string) but the function expects a boolean `watch`; change the call so the third
parameter is a boolean: for standard builds pass `false`, or if CLI watch
support exists pass `Boolean(args.watch)` (or `args.watch` directly) instead of
`target`, and ensure the `options` argument remains the last parameter.
解决了@achrinza/node-ipc不兼容node23的问题
修复了webpack持久缓存
Summary by CodeRabbit
Documentation
Bug Fixes
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.