perf(vite): use rolldown's replace plugin when applicable #33258
Conversation
|
|
packages/vite/package.json
Outdated
| "pathe": "^2.0.3", | ||
| "pkg-types": "^2.3.0", | ||
| "postcss": "^8.5.6", | ||
| "rolldown": ">=1.0.0-beta.36", |
There was a problem hiding this comment.
Made it open end for easier deduping.
We could also consider externalizing it?
| config.plugins!.push(replace(replaceOptions)) | ||
| // @ts-expect-error Rolldown-specific check | ||
| if(vite.rolldownVersion) { | ||
| const { replacePlugin } = await import('rolldown/experimental') |
There was a problem hiding this comment.
Will be moved over to a stable plugin soon™️ (rolldown/rolldown#5815)
WalkthroughThe Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/vite/src/vite.ts (2)
216-224: Skip installing the plugin when there’s nothing to replace.Avoid extra plugin work if no import.meta.* keys are defined.
Apply this diff:
nuxt.hook('vite:extendConfig', async (config) => { const replaceOptions = Object.create(null) for (const key in config.define!) { if (key.startsWith('import.meta.')) { replaceOptions[key] = config.define![key] } } + + if (Object.keys(replaceOptions).length === 0) { + return + }
227-232: Run the replace step as early as possible.To minimise other transforms seeing unreplaced import.meta.* tokens, consider moving the plugin to the front of the list.
Apply this diff:
- config.plugins!.push(replacePlugin(replaceOptions)) + config.plugins!.unshift(replacePlugin(replaceOptions)) ... - config.plugins!.push(replacePlugin({ ...replaceOptions, preventAssignment: true })) + config.plugins!.unshift(replacePlugin({ ...replaceOptions, preventAssignment: true }))Please double‑check plugin ordering doesn’t affect any downstream Vite/Rollup plugins in this repo.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
packages/vite/package.jsonis excluded by!**/package.jsonpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml,!pnpm-lock.yaml
📒 Files selected for processing (1)
packages/vite/src/vite.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Follow standard TypeScript conventions and best practices
Files:
packages/vite/src/vite.ts
🧠 Learnings (3)
📓 Common learnings
Learnt from: TheAlexLichter
PR: nuxt/nuxt#31812
File: packages/nuxt/src/components/plugins/islands-transform.ts:202-202
Timestamp: 2025-04-18T18:33:41.753Z
Learning: In Nuxt, using `rolldownVersion` (not `rollupVersion`) is intentional when detecting if rolldown-vite is being used, even though TypeScript may show an error because the property isn't in standard type definitions yet.
📚 Learning: 2025-04-18T18:33:41.753Z
Learnt from: TheAlexLichter
PR: nuxt/nuxt#31812
File: packages/nuxt/src/components/plugins/islands-transform.ts:202-202
Timestamp: 2025-04-18T18:33:41.753Z
Learning: In Nuxt, using `rolldownVersion` (not `rollupVersion`) is intentional when detecting if rolldown-vite is being used, even though TypeScript may show an error because the property isn't in standard type definitions yet.
Applied to files:
packages/vite/src/vite.ts
📚 Learning: 2024-11-05T15:22:54.759Z
Learnt from: GalacticHypernova
PR: nuxt/nuxt#26468
File: packages/nuxt/src/components/plugins/loader.ts:24-24
Timestamp: 2024-11-05T15:22:54.759Z
Learning: In `packages/nuxt/src/components/plugins/loader.ts`, the references to `resolve` and `distDir` are legacy code from before Nuxt used the new unplugin VFS and will be removed.
Applied to files:
packages/vite/src/vite.ts
🔇 Additional comments (2)
packages/vite/src/vite.ts (2)
216-233: Rolldown-aware replace plugin selection looks good.Dynamic import and conditional use of Rolldown’s native replace vs Rollup’s plugin is a sensible, low‑overhead switch.
225-226: Using vite.rolldownVersion behind a ts-expect-error is intentional.Matches prior Nuxt practice for Rolldown detection; keep it until types catch up, then drop the expect-error.
When TypeScript starts typing
rolldownVersion, please remove the@ts-expect-error(otherwise it will fail builds).
@nuxt/kit
nuxt
@nuxt/rspack-builder
@nuxt/schema
@nuxt/vite-builder
@nuxt/webpack-builder
commit: |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/vite/src/vite.ts (3)
215-222: Use a plain object for replaceOptions and short‑circuit when emptyAvoid a null‑prototype map (some plugins call obj.hasOwnProperty). Also skip importing/injecting a replacer when there’s nothing to replace.
- nuxt.hook('vite:extendConfig', async (config) => { - const replaceOptions = Object.create(null) + nuxt.hook('vite:extendConfig', async (config) => { + const replaceOptions: Record<string, unknown> = {} @@ - for (const key in config.define!) { + for (const key in (config.define ?? {})) { if (key.startsWith('import.meta.')) { replaceOptions[key] = config.define![key] } } + + if (Object.keys(replaceOptions).length === 0) { + return + }
224-231: Spread replaceOptions on the Rolldown path to avoid null‑proto edge cases; optional try/catch for resiliencePassing a spread produces a plain object (safer across implementations). Consider a try/catch to log/fallback if the dynamic import fails.
- if(vite.rolldownVersion) { - const { replacePlugin } = await import('rolldown/experimental') - config.plugins!.push(replacePlugin(replaceOptions)) + if (vite.rolldownVersion) { + const { replacePlugin } = await import('rolldown/experimental') + config.plugins!.push(replacePlugin({ ...replaceOptions })) } else { const { default: replacePlugin} = await import('@rollup/plugin-replace') config.plugins!.push(replacePlugin({ ...replaceOptions, preventAssignment: true })) }Optional (not shown in diff): wrap the rolldown branch in a try/catch and warn/fallback to the Rollup plugin on error.
215-231: Placement: ensure the replacer runs early enoughReplacements generally benefit from running before other transforms. Consider inserting earlier in the list (or wrapping as a Vite plugin with
enforce: 'pre') if you observe mismatches. Otherwise, please confirm current ordering is sufficient.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
packages/vite/package.jsonis excluded by!**/package.jsonpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml,!pnpm-lock.yaml
📒 Files selected for processing (1)
packages/vite/src/vite.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Follow standard TypeScript conventions and best practices
Files:
packages/vite/src/vite.ts
🧠 Learnings (3)
📓 Common learnings
Learnt from: TheAlexLichter
PR: nuxt/nuxt#31812
File: packages/nuxt/src/components/plugins/islands-transform.ts:202-202
Timestamp: 2025-04-18T18:33:41.753Z
Learning: In Nuxt, using `rolldownVersion` (not `rollupVersion`) is intentional when detecting if rolldown-vite is being used, even though TypeScript may show an error because the property isn't in standard type definitions yet.
📚 Learning: 2024-11-05T15:22:54.759Z
Learnt from: GalacticHypernova
PR: nuxt/nuxt#26468
File: packages/nuxt/src/components/plugins/loader.ts:24-24
Timestamp: 2024-11-05T15:22:54.759Z
Learning: In `packages/nuxt/src/components/plugins/loader.ts`, the references to `resolve` and `distDir` are legacy code from before Nuxt used the new unplugin VFS and will be removed.
Applied to files:
packages/vite/src/vite.ts
📚 Learning: 2025-04-18T18:33:41.753Z
Learnt from: TheAlexLichter
PR: nuxt/nuxt#31812
File: packages/nuxt/src/components/plugins/islands-transform.ts:202-202
Timestamp: 2025-04-18T18:33:41.753Z
Learning: In Nuxt, using `rolldownVersion` (not `rollupVersion`) is intentional when detecting if rolldown-vite is being used, even though TypeScript may show an error because the property isn't in standard type definitions yet.
Applied to files:
packages/vite/src/vite.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: code
🔇 Additional comments (2)
packages/vite/src/vite.ts (2)
224-226: LGTM on rolldown detectionUsing
vite.rolldownVersionbehind a// @ts-expect-erroris consistent with Nuxt’s current approach until types catch up.
224-231: Confirmed: rolldown's replacePlugin option shape matches @rollup/plugin-replaceIt accepts a plain replacement map or the { values: { ... } } form and exposes the preventAssignment flag — the current code is fine.
CodSpeed Performance ReportMerging #33258 will not alter performanceComparing Summary
Footnotes |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/vite/src/vite.ts (1)
6-6: Avoid identifier shadowing between Rollup and Rolldown replace pluginsUse a distinct name for the Rollup plugin to prevent confusion with the Rolldown export of the same name.
-import replacePlugin from '@rollup/plugin-replace' +import rollupReplace from '@rollup/plugin-replace'- config.plugins!.push(replacePlugin({ ...replaceOptions, preventAssignment: true })) + config.plugins!.push(rollupReplace({ ...replaceOptions, preventAssignment: true }))Also applies to: 230-230
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
packages/vite/package.jsonis excluded by!**/package.jsonpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml,!pnpm-lock.yaml
📒 Files selected for processing (1)
packages/vite/src/vite.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Follow standard TypeScript conventions and best practices
Files:
packages/vite/src/vite.ts
🧠 Learnings (3)
📓 Common learnings
Learnt from: TheAlexLichter
PR: nuxt/nuxt#31812
File: packages/nuxt/src/components/plugins/islands-transform.ts:202-202
Timestamp: 2025-04-18T18:33:41.753Z
Learning: In Nuxt, using `rolldownVersion` (not `rollupVersion`) is intentional when detecting if rolldown-vite is being used, even though TypeScript may show an error because the property isn't in standard type definitions yet.
📚 Learning: 2025-04-18T18:33:41.753Z
Learnt from: TheAlexLichter
PR: nuxt/nuxt#31812
File: packages/nuxt/src/components/plugins/islands-transform.ts:202-202
Timestamp: 2025-04-18T18:33:41.753Z
Learning: In Nuxt, using `rolldownVersion` (not `rollupVersion`) is intentional when detecting if rolldown-vite is being used, even though TypeScript may show an error because the property isn't in standard type definitions yet.
Applied to files:
packages/vite/src/vite.ts
📚 Learning: 2024-11-05T15:22:54.759Z
Learnt from: GalacticHypernova
PR: nuxt/nuxt#26468
File: packages/nuxt/src/components/plugins/loader.ts:24-24
Timestamp: 2024-11-05T15:22:54.759Z
Learning: In `packages/nuxt/src/components/plugins/loader.ts`, the references to `resolve` and `distDir` are legacy code from before Nuxt used the new unplugin VFS and will be removed.
Applied to files:
packages/vite/src/vite.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (16)
- GitHub Check: test-fixtures (windows-latest, built, vite, async, manifest-on, json, lts/-1)
- GitHub Check: test-fixtures (windows-latest, dev, vite, async, manifest-off, json, lts/-1)
- GitHub Check: test-fixtures (ubuntu-latest, built, rspack, async, manifest-on, json, lts/-1)
- GitHub Check: test-fixtures (windows-latest, dev, vite, async, manifest-on, json, lts/-1)
- GitHub Check: test-fixtures (ubuntu-latest, dev, vite, async, manifest-off, json, lts/-1)
- GitHub Check: test-fixtures (ubuntu-latest, dev, vite, default, manifest-on, json, lts/-1)
- GitHub Check: test-fixtures (ubuntu-latest, built, vite, async, manifest-on, json, lts/-1)
- GitHub Check: test-fixtures (ubuntu-latest, built, vite, async, manifest-off, json, lts/-1)
- GitHub Check: test-fixtures (ubuntu-latest, dev, vite, default, manifest-off, json, lts/-1)
- GitHub Check: test-fixtures (ubuntu-latest, built, vite, async, manifest-on, js, lts/-1)
- GitHub Check: test-fixtures (ubuntu-latest, dev, vite, async, manifest-on, json, lts/-1)
- GitHub Check: typecheck (windows-latest, bundler)
- GitHub Check: release-pkg-pr-new
- GitHub Check: typecheck (ubuntu-latest, bundler)
- GitHub Check: test-benchmark
- GitHub Check: test-size
🔇 Additional comments (2)
packages/vite/src/vite.ts (2)
133-141: LGTM: rolldown-specific build.watch handlingGood conditional handling for rolldown (dropping chokidar options). This aligns with prior Nuxt usage of vite.rolldownVersion.
216-232: Harden plugin injection and add TS guard for 'rolldown/experimental' importRepo search found no
declare module "rolldown/experimental"— add null guards for config.plugins/config.define and add a// @ts-expect-errorabove the experimental import (or add an ambient module declaration).Apply within this block:
- nuxt.hook('vite:extendConfig', async (config) => { - const replaceOptions = Object.create(null) + nuxt.hook('vite:extendConfig', async (config) => { + config.plugins ||= [] + const replaceOptions: Record<string, unknown> = Object.create(null) - for (const key in config.define!) { + const define = config.define || {} + for (const key in define) { - if (key.startsWith('import.meta.')) { - replaceOptions[key] = config.define![key] + if (key.startsWith('import.meta.')) { + replaceOptions[key] = define[key] } } // @ts-expect-error Rolldown-specific check if (vite.rolldownVersion) { - const { replacePlugin } = await import('rolldown/experimental') + // @ts-expect-error: experimental module has no types (yet) + const { replacePlugin } = await import('rolldown/experimental') config.plugins!.push(replacePlugin(replaceOptions)) } else { - config.plugins!.push(replacePlugin({ ...replaceOptions, preventAssignment: true })) + // Optional: lazy-load to avoid loading the Rollup plugin when on rolldown + // const { default: rollupReplace } = await import('@rollup/plugin-replace') + // config.plugins!.push(rollupReplace({ ...replaceOptions, preventAssignment: true })) + config.plugins!.push(replacePlugin({ ...replaceOptions, preventAssignment: true })) } })
🔗 Linked issue
Related to #30654
📚 Description
When
rolldown-viteis detected, use Rolldown's internal replace plugin instead of the rollup plugin.