fix(nuxt): use rollup to calculate island component filenames #32421
Conversation
|
|
WalkthroughThe changes refactor the setup and implementation of the ComponentsChunkPlugin used in the componentIslands experimental feature. The plugin logic is consolidated: instead of separate handling for client and server plugins and explicit hooks for Vite config extension, the plugin is now conditionally added as a Vite plugin when selectiveClient is enabled and the builder is Vite. The ComponentsChunkPlugin itself is rewritten to return an array of two Vite plugins, handling chunk emission and mapping generation more directly within Vite's lifecycle. File system writes are removed, and the mapping is now provided via a virtual module. No exported or public entity declarations were removed; a new dev option was added. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (4)
✨ Finishing Touches
🧪 Generate Unit Tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
packages/nuxt/src/components/plugins/islands-transform.ts (1)
234-241: Race condition: server build may write an empty chunk map
idsis populated in the client build; the server build (where this code runs) can start first, producing an emptycomponent-chunk.mjs. Result: runtime cannot resolve islands.Fix: derive the map locally (or delay write until all ids exist). Minimal change:
- buildStart () { + generateBundle () { // ensure map is up-to-date even if client build ran after server build start if (ids.size === 0) { for (const c of options.getComponents()) { if ((c.mode === 'client' || c.mode === 'all') && !ids.has(c.pascalName)) { const file = join(finalBuildAssetsDir, `${kebabCase(c.pascalName)}.*.mjs`) ids.set(c.pascalName, file) } } } writeFileSync( join(useNuxt().options.buildDir, 'component-chunk.mjs'), `export default {${Array.from(ids.entries()).map(([n, i]) => `${JSON.stringify(n)}: ${JSON.stringify('/' + i)}`).join(',\n')}}`, ) }This removes the ordering dependency between the two builds.
🧹 Nitpick comments (2)
packages/nuxt/src/components/plugins/islands-transform.ts (2)
189-192: Normalisation helper can be simplified & made saferThe current two-step construction is a little brittle (e.g.
'','.', superfluous double slashes). Usingjoinplusreplace(/^\/+|\/+$/g, '')results in cleaner, single-point logic:-const normalizedBuildAssetsDir = options.buildAssetsDir.startsWith('/') ? options.buildAssetsDir.slice(1) : options.buildAssetsDir + '/' -const finalBuildAssetsDir = normalizedBuildAssetsDir.endsWith('/') ? normalizedBuildAssetsDir : normalizedBuildAssetsDir + '/' +const finalBuildAssetsDir = (options.buildAssetsDir || '_nuxt') + .replace(/^\/+|\/+$/g, '') // trim both ends + .concat('/') // ensure trailing slash
204-208: Blocking I/O inbuildStart– switch to async to avoid event-loop stalls
readFileSyncis executed once per client/all component. Large component sets will freeze the event loop during bundle start-up. Vite’sthis.emitFileis already sync-friendly, so switching tofs.promises.readFilekeeps the build responsive:-const componentsContent = readFileSync(component.filePath, 'utf-8') +const componentsContent = await fs.promises.readFile(component.filePath, 'utf-8')Wrap the parent handler in
async(Rollup tolerates promises inbuildStart).
[note: minor impact in small projects, but projects with thousands of islands will appreciate it]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/nuxt/src/components/module.ts(1 hunks)packages/nuxt/src/components/plugins/islands-transform.ts(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: codeql (javascript-typescript)
- GitHub Check: build
- GitHub Check: codeql (actions)
- GitHub Check: code
🔇 Additional comments (1)
packages/nuxt/src/components/module.ts (1)
252-252: Path argument looks good – just confirm it’s always defined
nuxt.options.app.buildAssetsDiris forwarded unguarded. Nuxt sets a default ("/_nuxt/"), but if a user explicitly sets it tofalse/undefinedthis will now throw at runtime inside the plugin.
A quick null-coalescing fallback avoids any accidental mis-configuration:-const chunk = ComponentsChunkPlugin({ getComponents, buildAssetsDir: nuxt.options.app.buildAssetsDir }) +const chunk = ComponentsChunkPlugin({ + getComponents, + buildAssetsDir: nuxt.options.app.buildAssetsDir ?? '/_nuxt/' +})
@nuxt/kit
nuxt
@nuxt/rspack-builder
@nuxt/schema
@nuxt/vite-builder
@nuxt/webpack-builder
commit: |
CodSpeed Performance ReportMerging #32421 will not alter performanceComparing Summary
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/nuxt/src/components/plugins/islands-transform.ts (1)
204-204: Fix the async/await syntax error.The
awaitkeyword cannot be used in a non-async function. ThebuildStartmethod needs to be made async.- vite: { - buildStart () { + vite: { + async buildStart () {This matches the pattern suggested in the past review comment and resolves the static analysis error.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/nuxt/src/components/plugins/islands-transform.ts(3 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/nuxt/src/components/plugins/islands-transform.ts
[error] 204-204: await is only allowed within async functions and at the top levels of modules.
(parse)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: codeql (javascript-typescript)
- GitHub Check: codeql (actions)
🔇 Additional comments (3)
packages/nuxt/src/components/plugins/islands-transform.ts (3)
2-3: LGTM! New imports support the enhanced functionality.The added imports (
readFileSync,hash,kebabCase) are appropriate for the content-based hashing and improved file naming strategy.Also applies to: 11-11
183-183: Good addition for configurable build assets directory.Adding
buildAssetsDirto the options type aligns with the PR objective to eliminate hardcoded paths.
207-207: Excellent improvement to content-based hashing.The new file naming strategy using
kebabCase(component.pascalName)andhash(componentsContent)effectively addresses the PR objective of dynamic hashing that updates with component changes, replacing the previous hardcoded approach.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/nuxt/src/components/plugins/islands-transform.ts (1)
186-196: Consider removing the fallback plugin implementation.The early return plugin that exports an empty object may be unnecessary complexity. If selective client mode is disabled, the consuming code should handle the absence of chunk mappings gracefully.
Consider simplifying to:
-export const ComponentsChunkPlugin = (options: ChunkPluginOptions) => createUnplugin((_, meta) => { - if (meta.framework !== 'vite' || !options.selectiveClient) { - return { - name: 'nuxt:components-chunk', - load (id) { - if (id === COMPONENT_CHUNK_ID) { - return `export default {}` - } - }, - } - } +export const ComponentsChunkPlugin = (options: ChunkPluginOptions) => createUnplugin((_, meta) => { + if (meta.framework !== 'vite' || !options.selectiveClient) { + return { name: 'nuxt:components-chunk' } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/nuxt/src/app/components/nuxt-teleport-island-component.ts(1 hunks)packages/nuxt/src/components/module.ts(1 hunks)packages/nuxt/src/components/plugins/islands-transform.ts(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/nuxt/src/app/components/nuxt-teleport-island-component.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/nuxt/src/components/module.ts
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.ts`: Follow standard TypeScript conventions and best practices
**/*.ts: Follow standard TypeScript conventions and best practices
packages/nuxt/src/components/plugins/islands-transform.ts
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: codeql (javascript-typescript)
- GitHub Check: codeql (actions)
- GitHub Check: build
- GitHub Check: code
🔇 Additional comments (7)
packages/nuxt/src/components/plugins/islands-transform.ts (7)
7-7: LGTM! Import addition supports the new implementation.The knitwork utilities are correctly imported and used later for generating JavaScript object literals and strings in the virtual module implementation.
177-181: LGTM! Type definition properly extended.The addition of
devandselectiveClientboolean properties toChunkPluginOptionsis well-typed and supports the enhanced plugin configuration.
183-184: LGTM! Virtual module constants are well-defined.The constants for virtual module IDs follow Nuxt conventions and clearly separate the public ID from the resolved internal ID.
198-217: LGTM! Client plugin chunk emission logic is sound.The client plugin correctly:
- Applies only to production client builds
- Emits chunks for components with file paths that aren't server-only
- Uses appropriate chunk naming convention with Pascal case
- Preserves strict signatures for proper tree-shaking
218-233: LGTM! Bundle generation handling is correct.The
generateBundlelogic properly:
- Tracks emitted chunk filenames
- Maps component names to generated filenames
- Marks component chunks as non-entry points to prevent unwanted code splitting
235-245: LGTM! Virtual module resolution is properly implemented.The server plugin correctly resolves the virtual module ID using pre-order resolution, which ensures it takes precedence over other resolvers.
246-268: LGTM! Virtual module loading handles both dev and production modes correctly.The implementation properly:
- Uses
@fs/prefix for development file system access- Uses
/prefix for production chunk paths (appropriate for public asset URLs)- Leverages knitwork utilities to generate safe JavaScript object literals
- Filters out server-only components consistently
The path prefixing logic aligns with Vite's asset handling conventions.
|
ported some refactors from #32461 and moved more logic into the vite plugin. basically we shouldn't be hashing/calculating file names at all as that's something that rollup can happily do for us. maybe in future we can also use do look to see if you spot any ways to improve (cc: @huang-julien) |
…g (#32411)
🔗 Linked issue
resolves #32411
📚 Description
Fix the harcoded build path in componentIsland and dynamic hashing for every change in components.
Example:
initial output 'http://localhost:3000/07332f681020/_nuxt/nJKChgM0-iThgYsLbhDKxBipcnkOFskrNVzUzZPaAts.mjs'
after fix output 'http://localhost:3000/{BuildAssetsDir}/{component-kebabCase}.{[hash]}.mjs'