refactor(build): extract shared build helpers into build-shared.js #14147
Conversation
WalkthroughThe pull request consolidates duplicated build configuration logic from Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ 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 |
❌ Deploy Preview for vue-sfc-playground failed. Why did it fail? →
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
scripts/build-shared.js (2)
113-166: Double‑check that shared define semantics (SSR/BROWSER/compat) match previous behaviorThe consolidated
resolveDefineshelper looks consistent and expressive, but it centralizes several subtle flags (__BROWSER__,__SSR__,__COMPAT__, feature flags) that were previously inlined:
__BROWSER__now depends on(isGlobalBuild || isBrowserESMBuild || isBundlerESMBuild) && !enableNonBrowserBranches.__SSR__is derived solely from!isGlobalBuild.Given how widely these flags are used in the runtime/compiler, it’s worth confirming these expressions exactly preserve the prior Rollup logic, especially for
esm-browserandesm-bundlerformats and compat builds.
190-205: Entry resolution for compat vs non‑compat packages appears correct
resolveEntryFiledistinguishes:
- runtime vs full builds via
/runtime$/,- ESM browser/bundler compat builds (
src/esm-runtime.ts/src/esm-index.ts),- all others via
src/runtime.ts/src/index.ts.This matches how compat entries are typically specialized while keeping the API simple. Consider adding a brief comment noting which formats are expected to use the ESM compat entries to help future maintainers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
rollup.config.js(3 hunks)scripts/build-shared.js(1 hunks)scripts/dev.js(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
scripts/dev.js (1)
scripts/build-shared.js (6)
resolveOutputFormat(173-177)resolvePostfix(184-188)external(41-41)resolveExternal(17-77)defines(143-163)resolveDefines(124-166)
rollup.config.js (1)
scripts/build-shared.js (3)
resolveEntryFile(196-206)resolveDefines(124-166)resolveCJSIgnores(97-111)
🔇 Additional comments (8)
scripts/build-shared.js (3)
17-77: External resolution logic is coherent and matches the described build modesThe
resolveExternalimplementation cleanly separates:
- global/browser/compat builds (early-return tree‑shaken deps only) and
- cjs/esm‑bundler builds with dependency/peerDependency externals and compiler‑sfc extras (including consolidate deps and Node/bundler‑only packages).
This aligns with the intent to centralize external handling and should be reusable across both Rollup and dev builds without obvious regressions.
79-111: Graceful consolidate dependency handling and CJS ignore list look solid
getConsolidateDepssafely falls back to[]when@vue/consolidateis absent, andresolveCJSIgnorescomposes the same consolidate‑derived list with additional Node‑only packages forcompiler-sfc. This matches how the externals are built and avoids hard failures when consolidate isn't installed.
168-188: Output format and postfix helpers correctly capture existing conventions
resolveOutputFormatandresolvePostfixnicely encode:
- global/global‑runtime →
iife- cjs →
cjs- other formats →
esm*-runtime→runtime.<baseFormat>This matches the naming scheme used in
rollup.config.jsand is a good centralization; no issues spotted.scripts/dev.js (2)
14-19: Shared output format/postfix helpers are correctly integratedThe dev script now uses
resolveOutputFormat(format)andresolvePostfix(format)instead of duplicating logic, which keeps esbuild output and filenames aligned with Rollup’s conventions (global*→iife,*-runtime→runtime.<base>). No functional issues here.Also applies to: 51-53
100-107: Shared define resolution keeps dev and Rollup flags consistentUsing
resolveDefines({ pkg, format, target, prod, commit: 'dev' })and passing the result directly into esbuild’sdefineunifies dev‑time flags with the Rollup build pipeline. This reduces drift in things like__VERSION__,__DEV__, and feature flags between environments; the hardcodedcommit: 'dev'also matches prior dev behavior.Also applies to: 120-120
rollup.config.js (3)
18-23: Entry resolution now correctly centralized viaresolveEntryFileImporting
resolveEntryFileand using:const entryFile = resolveEntryFile(format, isCompatPackage) // ... input: resolve(entryFile),is a clean extraction of the previously inlined entry selection logic (runtime vs full, compat ESM vs non‑compat). This should make future tweaks to compat/runtime entry mapping much easier.
Also applies to: 164-166, 260-262
167-188: Shared define generation is wired correctly with existing override behavior
resolveDefine()now seeds replacements fromresolveDefines({ pkg, format, target, prod, version: masterVersion, commit }), and the later loop still allows environment variables to override any of those keys before passing them into the esbuild plugin.This preserves the previous override semantics while centralizing the core define logic in
build-shared.js.Also applies to: 274-280
227-238: External resolution and CJS ignores are cleanly delegated to shared helpers
resolveExternal()now forwardspkg,format,process.env.TARGET, and the computedisGlobalBuild,isBrowserESMBuild,isCompatPackage, andpackageOptionsintoresolveExternalShared, so Rollup and dev builds share the same external rules.resolveNodePlugins()derivescjsIgnoresviaresolveCJSIgnores(process.env.TARGET)and feeds that into the commonjs plugin’signoreoption, matching the externals logic forcompiler-sfc.This consolidation removes a lot of bespoke Rollup‑only wiring without changing behavior in an obvious way.
Also applies to: 240-255
| // resolve externals using shared function | ||
| const external = inlineDeps | ||
| ? [] | ||
| : resolveExternal({ | ||
| pkg, | ||
| format, | ||
| target, | ||
| isGlobalBuild: format === 'global', | ||
| isBrowserESMBuild: format.includes('esm-browser'), | ||
| isCompatPackage: target === 'vue-compat', | ||
| packageOptions: pkg.buildOptions || {}, | ||
| }) |
There was a problem hiding this comment.
Align dev isGlobalBuild with Rollup for runtime formats
The dev external resolution now delegates to resolveExternal, but:
isGlobalBuild: format === 'global',while Rollup derives isGlobalBuild from /global/.test(format), which also treats global-runtime as a global build.
If scripts/dev.js is ever invoked with format=global-runtime, externals for that mode will differ from Rollup (notably around treeShakenDeps handling). To keep dev and production behavior in sync, consider switching this to the same predicate, e.g.:
- isGlobalBuild: format === 'global',
+ isGlobalBuild: format.startsWith('global'),(or mirroring whatever check Rollup uses).
🤖 Prompt for AI Agents
In scripts/dev.js around lines 71 to 82, the dev external resolution uses a
strict equality check isGlobalBuild: format === 'global' which diverges from
Rollup's /global/.test(format) (so formats like 'global-runtime' are treated
differently); change the predicate to mirror Rollup (e.g. use a regex test like
/global/.test(format>)) so isGlobalBuild is true for any format containing
"global", ensuring externals and treeShakenDeps behavior match Rollup's
runtime/global handling.
Summary
This PR refactors the build system by extracting duplicated logic from dev.js and Rollup’s build-shared.js into a new shared helper module scripts/build-shared.js.
What this PR does
scripts/build-shared.jsthat exports:resolveDefinesresolveExternalresolveEntryFileresolveOutputFormatresolvePostfixdev.jsto use the new shared helper functions.Benefits
I ran tests and simple benchmarks on my machine and observed no noticeable performance difference compared to the current implementation.
This refactor addresses an existing TODO in the build scripts related to extracting shared build logic (line 73) .
scripts/dev.jsSummary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.