feat(nuxt): lazy hydration macros without auto-imports #33037
Conversation
|
|
WalkthroughReplaces regex-based macro handling with an AST-driven transform that finds Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes ✨ 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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
@nuxt/kit
nuxt
@nuxt/rspack-builder
@nuxt/schema
@nuxt/vite-builder
@nuxt/webpack-builder
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
packages/nuxt/src/components/plugins/lazy-hydration-macro-transform.ts (4)
20-20: Make the import-path regex resilient to whitespace and quote-capture safely.Your new
COMPONENT_IMPORTworks for the happy path but will miss cases such asimport ( '...' )(extra whitespace) and forces consumers to remember the index of the captured group. Tighten it to tolerate whitespace and capture the quote so we can reliably reference the path by index 2, then adapt the usages accordingly.-const COMPONENT_IMPORT = /import\(["']([^'"]+)["']\)/ +const COMPONENT_IMPORT = /import\s*\(\s*(['"])([^'"]+)\1\s*\)/ - const componentImportPath = matchedString.match(COMPONENT_IMPORT) - if (!componentImportPath || !componentImportPath[1]) { + const componentImportPath = matchedString.match(COMPONENT_IMPORT) + const importPath = componentImportPath?.[2] + if (!importPath) { s.remove(startIndex, endIndex) continue } - component = { - filePath: componentImportPath[1], + component = { + filePath: importPath, pascalName: pascalCase(name), } as ComponentAlso applies to: 80-85, 88-88
77-92: Avoid early bail-out when component-name parsing fails; derive name from the import path.At present, if
COMPONENT_NAMEfails (Line 70–75), you remove the macro before attempting the new explicit-import fallback. That means valid macros with slightly atypical filenames or query strings would still be dropped. Consider deferring the name derivation until after tryingCOMPONENT_IMPORT, and if needed, deriving the base name from the import path:Example (illustrative snippet, not a diff):
const importMatch = matchedString.match(COMPONENT_IMPORT) const importPath = importMatch?.[2] const fileBase = importPath?.split(/[?#]/)[0].split(/[\\/]/).pop() // e.g. 'MyComponent.vue' const derivedName = fileBase?.replace(/\.\w+$/, '') const name = derivedName || componentNameMatch?.[1] if (!name) { s.remove(startIndex, endIndex); continue }This keeps the new behaviour working even when
COMPONENT_NAMEdoesn’t match but an explicit import path is present.
86-91: Be explicit aboutexportto avoid subtlety and future regressions.You rely on
component.export ?? 'default'later; for the synthetic component you create here, setexport: 'default'explicitly so the intent is self-documenting and resilient to future changes.component = { filePath: importPath, pascalName: pascalCase(name), + export: 'default', } as Component
97-97: Minor readability tweak: precompute the parameter path.Building the JSON directly inline makes the ternary a bit harder to scan. Precompute and stringify once; it also makes it simpler to toggle behaviour in future.
Example (illustrative snippet):
const paramPath = shouldUseRelativePathAsParam ? relativePath : component.filePath const replacement = `const ${variableName} = __${replaceFunctionName}(${JSON.stringify(paramPath)}, ${dynamicImport})`packages/nuxt/test/lazy-hydration-macro-transform.test.ts (2)
51-69: Add parity tests for TS and JSX when components aren’t auto-imported.To prevent regressions across syntaxes, mirror this SFC test with TS and JSX sources (similar to existing auto-import tests). This ensures the macro fallback behaves identically regardless of the authoring style.
Happy to draft those tests if helpful.
154-170: Helper component map: small alignment with runtime expectations.The shape looks correct for tests. Two small nits you may consider:
- Set
export: 'default'explicitly on the test component to mirror production defaults.- If you add tests for named exports later, expose
export: 'Named'here to validate the.then(c => c[export] || c)path end-to-end.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/nuxt/src/components/plugins/lazy-hydration-macro-transform.ts(2 hunks)packages/nuxt/test/lazy-hydration-macro-transform.test.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Follow standard TypeScript conventions and best practices
Files:
packages/nuxt/test/lazy-hydration-macro-transform.test.tspackages/nuxt/src/components/plugins/lazy-hydration-macro-transform.ts
**/*.{test,spec}.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Write unit tests for core functionality using
vitest
Files:
packages/nuxt/test/lazy-hydration-macro-transform.test.ts
🧠 Learnings (2)
📚 Learning: 2024-12-12T12:36:34.871Z
Learnt from: huang-julien
PR: nuxt/nuxt#29366
File: packages/nuxt/src/app/components/nuxt-root.vue:16-19
Timestamp: 2024-12-12T12:36:34.871Z
Learning: In `packages/nuxt/src/app/components/nuxt-root.vue`, when optimizing bundle size by conditionally importing components based on route metadata, prefer using inline conditional imports like:
```js
const IsolatedPage = route?.meta?.isolate ? defineAsyncComponent(() => import('#build/isolated-page.mjs')) : null
```
instead of wrapping the import in a computed property or importing the component unconditionally.
Applied to files:
packages/nuxt/src/components/plugins/lazy-hydration-macro-transform.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/nuxt/src/components/plugins/lazy-hydration-macro-transform.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). (20)
- GitHub Check: test-fixtures (windows-latest, built, rspack, default, manifest-on, json, lts/-1)
- GitHub Check: test-fixtures (windows-latest, dev, vite, async, manifest-on, json, lts/-1)
- GitHub Check: test-fixtures (windows-latest, built, vite, default, manifest-off, json, lts/-1)
- GitHub Check: test-fixtures (windows-latest, dev, vite, default, 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, vite, default, manifest-on, json, lts/-1)
- GitHub Check: test-fixtures (windows-latest, dev, vite, default, manifest-off, json, lts/-1)
- GitHub Check: test-fixtures (ubuntu-latest, built, vite, default, manifest-off, json, lts/-1)
- GitHub Check: test-fixtures (ubuntu-latest, built, webpack, async, manifest-on, json, lts/-1)
- GitHub Check: test-fixtures (ubuntu-latest, dev, vite, default, manifest-off, json, lts/-1)
- GitHub Check: test-fixtures (ubuntu-latest, built, rspack, 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, built, 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, dev, vite, async, manifest-on, json, lts/-1)
- GitHub Check: test-fixtures (ubuntu-latest, dev, vite, async, manifest-on, js, lts/-1)
- GitHub Check: test-benchmark
- GitHub Check: release-pkg-pr-new
- GitHub Check: typecheck (windows-latest, bundler)
- GitHub Check: typecheck (ubuntu-latest, bundler)
🔇 Additional comments (1)
packages/nuxt/test/lazy-hydration-macro-transform.test.ts (1)
51-69: Great addition covering the non-auto-import path.The new test accurately validates the explicit-import fallback and checks both the import of the runtime creator and the transformed callsite. This directly addresses the regression described in the PR.
CodSpeed Performance ReportMerging #33037 will not alter performanceComparing Summary
|
danielroe
changed the title
danielroe
changed the title
|
Thank you so much for bringing up and fixing this issue. After looking into it a bit, I’m not entirely sure if using a regular expression alone is robust enough. I tried re-implementing it with const TryComponent = defineLazyHydrationComponent(
'interaction',
() => import('@/components/atomics/TryComponent').then(c => c.TryComponent),
)Would it be possible for me to give this approach a try? |
|
please do! 🙏 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
test/bundle.test.ts (1)
98-98: Avoid brittle byte-size snapshots; assert within a bounded range insteadA 1k delta is expected after adding the macro runtime import. To reduce CI flakiness across platforms, assert a range instead of an exact "549k".
- expect.soft(roundToKilobytes(serverStats.totalBytes)).toMatchInlineSnapshot(`"549k"`) + const kb = serverStats.totalBytes / 1024 + expect.soft(kb).toBeGreaterThanOrEqual(545) + expect.soft(kb).toBeLessThanOrEqual(555)test/fixtures/basic/app/pages/lazy-import-components/delayed-hydration/macro/index.vue (1)
46-51: LGTM: switch to alias-based importsUsing '~/components/DelayedComponent.vue' aligns with the new alias-aware transform. Fixture reads clearer and matches the test expectations.
To DRY up the repeated loader, consider:
<script setup lang="ts"> -const LazyVisibleDelayedComponentMacro = defineLazyHydrationComponent('visible', () => import('~/components/DelayedComponent.vue')) +const loadDelayed = () => import('~/components/DelayedComponent.vue') +const LazyVisibleDelayedComponentMacro = defineLazyHydrationComponent('visible', loadDelayed) const LazyIdleDelayedComponentMacro = defineLazyHydrationComponent('idle', loadDelayed) const LazyInteractionDelayedComponentMacro = defineLazyHydrationComponent('interaction', loadDelayed) const LazyMediaQueryDelayedComponentMacro = defineLazyHydrationComponent('mediaQuery', loadDelayed) const LazyIfDelayedComponentMacro = defineLazyHydrationComponent('if', loadDelayed) const LazyNeverDelayedComponentMacro = defineLazyHydrationComponent('never', loadDelayed) </script>packages/nuxt/test/lazy-hydration-macro-transform.test.ts (2)
51-69: Add coverage for named exports and other loader shapesGreat addition for the non-auto-imported path. Please extend coverage to ensure the AST matcher handles:
- import(...).then(m => m.NamedExport)
- async arrow with await import(...)
- block-bodied arrow returning import(...)
+ it('supports loader returning a named export', async () => { + const sfc = ` + <script setup> + const C = defineLazyHydrationComponent('interaction', () => import('~/components/MyComponent.vue').then(m => m.MyComponent)) + </script><template><C/></template>` + const code = await transform(sfc, '/pages/index.vue', true) + expect(code).toContain(`createLazyInteractionComponent("components/MyComponent.vue", () => import('~/components/MyComponent.vue').then(m => m.MyComponent))`) + }) + + it('supports await import in async arrow', async () => { + const sfc = ` + <script setup> + const C = defineLazyHydrationComponent('idle', async () => await import('~/components/MyComponent.vue')) + </script><template><C/></template>` + const code = await transform(sfc, '/pages/index.vue', true) + expect(code).toContain(`createLazyIdleComponent("components/MyComponent.vue", async () => await import('~/components/MyComponent.vue'))`) + }) + + it('supports block-bodied arrow returning import', async () => { + const sfc = ` + <script setup> + const C = defineLazyHydrationComponent('visible', () => { return import('~/components/MyComponent.vue') }) + </script><template><C/></template>` + const code = await transform(sfc, '/pages/index.vue', true) + expect(code).toContain(`createLazyVisibleComponent("components/MyComponent.vue", () => { return import('~/components/MyComponent.vue') })`) + })
154-171: Helper changes LGTM; minor test harness nits
- noComponents flag is a clean way to exercise the non-discovery path.
- Alias map { '~/': '/' } mirrors module wiring.
Consider asserting that the generated import of factories is deduplicated (single import even with multiple macros), to lock in that invariant.
Also applies to: 199-204
packages/nuxt/src/components/plugins/lazy-hydration-macro-transform.ts (2)
61-96: Broaden supported loader shapes (await/block-bodied arrows)Currently only expression-bodied arrows with ImportExpression or import(...).then(...) are handled. Real-world code often uses:
- async () => await import('...')
- () => { return import('...') }
- if (loaderArgument?.type !== 'ArrowFunctionExpression') { return } - if (loaderArgument.body.type === 'BlockStatement') { return } + if (loaderArgument?.type !== 'ArrowFunctionExpression') { return } + + // Extract the expression to analyse (supports expression body or `return` in block) + let bodyExpr: Expression | null = null + if (loaderArgument.body.type === 'BlockStatement') { + const ret = loaderArgument.body.body.find(s => s.type === 'ReturnStatement' && s.argument) as any + bodyExpr = ret?.argument ?? null + } else { + bodyExpr = loaderArgument.body + } + if (!bodyExpr) { return } - if (loaderArgument.body.type === 'ImportExpression') { - importExpression = loaderArgument.body - importLiteral = loaderArgument.body.source + if (bodyExpr.type === 'AwaitExpression' && bodyExpr.argument?.type) { + // unwrap `await import(...)` + bodyExpr = bodyExpr.argument as Expression + } + + if (bodyExpr.type === 'ImportExpression') { + importExpression = bodyExpr + importLiteral = bodyExpr.source - } else if ( - loaderArgument.body.type === 'CallExpression' && - loaderArgument.body.callee.type === 'MemberExpression' && - loaderArgument.body.callee.object.type === 'ImportExpression' + } else if ( + bodyExpr.type === 'CallExpression' && + bodyExpr.callee.type === 'MemberExpression' && + bodyExpr.callee.object.type === 'ImportExpression' ) { - importExpression = loaderArgument.body.callee.object + importExpression = bodyExpr.callee.object importLiteral = importExpression.source } else { return } - const originalLoader = code.slice(loaderArgument.start, loaderArgument.end) + const originalLoader = code.slice(loaderArgument.start, loaderArgument.end)Also applies to: 104-109
137-145: Normalise alias keys to handle missing trailing slashesIf an alias key lacks a trailing slash, startsWith may incorrectly match prefixes. Normalise keys before comparison.
-function resolveAliases (aliases: [string, string][], filePath: string) { +function resolveAliases (aliases: [string, string][], filePath: string) { for (const [alias, target] of aliases) { - if (!filePath.startsWith(alias)) { continue } - const rest = filePath.slice(alias.length).replace(/^[/\\]+/, '') + const a = alias.endsWith('/') ? alias : alias + '/' + if (!filePath.startsWith(a)) { continue } + const rest = filePath.slice(a.length).replace(/^[/\\]+/, '') return resolve(target, rest) } return filePath }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
packages/nuxt/src/components/module.ts(1 hunks)packages/nuxt/src/components/plugins/lazy-hydration-macro-transform.ts(3 hunks)packages/nuxt/test/lazy-hydration-macro-transform.test.ts(4 hunks)test/bundle.test.ts(1 hunks)test/fixtures/basic/app/pages/lazy-import-components/delayed-hydration/macro/index.vue(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Follow standard TypeScript conventions and best practices
Files:
test/bundle.test.tspackages/nuxt/src/components/module.tspackages/nuxt/src/components/plugins/lazy-hydration-macro-transform.tspackages/nuxt/test/lazy-hydration-macro-transform.test.ts
**/*.{test,spec}.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Write unit tests for core functionality using
vitest
Files:
test/bundle.test.tspackages/nuxt/test/lazy-hydration-macro-transform.test.ts
**/*.vue
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use
<script setup lang="ts">and the composition API when creating Vue components
Files:
test/fixtures/basic/app/pages/lazy-import-components/delayed-hydration/macro/index.vue
🧠 Learnings (3)
📚 Learning: 2024-12-12T12:36:34.871Z
Learnt from: huang-julien
PR: nuxt/nuxt#29366
File: packages/nuxt/src/app/components/nuxt-root.vue:16-19
Timestamp: 2024-12-12T12:36:34.871Z
Learning: In `packages/nuxt/src/app/components/nuxt-root.vue`, when optimizing bundle size by conditionally importing components based on route metadata, prefer using inline conditional imports like:
```js
const IsolatedPage = route?.meta?.isolate ? defineAsyncComponent(() => import('#build/isolated-page.mjs')) : null
```
instead of wrapping the import in a computed property or importing the component unconditionally.
Applied to files:
test/fixtures/basic/app/pages/lazy-import-components/delayed-hydration/macro/index.vuepackages/nuxt/test/lazy-hydration-macro-transform.test.ts
📚 Learning: 2025-07-18T16:46:07.446Z
Learnt from: CR
PR: nuxt/nuxt#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-18T16:46:07.446Z
Learning: Applies to **/*.vue : Use `<script setup lang="ts">` and the composition API when creating Vue components
Applied to files:
test/fixtures/basic/app/pages/lazy-import-components/delayed-hydration/macro/index.vue
📚 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/nuxt/src/components/module.tspackages/nuxt/src/components/plugins/lazy-hydration-macro-transform.ts
🔇 Additional comments (4)
packages/nuxt/src/components/module.ts (1)
233-237: Pass-through of alias map to macro plugin is correctForwarding nuxt.options.alias enables proper path resolution in the macro transform. No further changes needed here.
packages/nuxt/test/lazy-hydration-macro-transform.test.ts (1)
41-48: Snapshot updates look consistent with alias-based importsThe transformed lines now use import('~/components/...') and the first arg resolves to "components/MyComponent.vue". These match the new plugin behaviour.
Also applies to: 102-108, 143-149
packages/nuxt/src/components/plugins/lazy-hydration-macro-transform.ts (2)
20-28: Factory mapping reads clearly and covers all strategiesMapping is explicit and future-proof.
34-35: Longest-first alias ordering is the right approachPrevents shorter aliases shadowing longer, more specific ones.
packages/nuxt/src/components/plugins/lazy-hydration-macro-transform.ts
Outdated
Show resolved
Hide resolved
|
I second that 👍 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/nuxt/src/components/plugins/lazy-hydration-macro-transform.ts (1)
133-135: Consider using a more specific type guard.The
isStringLiteralfunction usesanytype and checks fornode.type === 'Literal'. This could be more type-safe.Consider using the actual AST node type from oxc-parser:
-function isStringLiteral (node: any): node is StringLiteral { - return !!node && node.type === 'Literal' && typeof node.value === 'string' +function isStringLiteral (node: Expression | null | undefined): node is StringLiteral { + return !!node && node.type === 'Literal' && typeof (node as any).value === 'string' }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
packages/nuxt/src/components/module.ts(1 hunks)packages/nuxt/src/components/plugins/lazy-hydration-macro-transform.ts(3 hunks)packages/nuxt/test/lazy-hydration-macro-transform.test.ts(4 hunks)test/fixtures/basic/app/pages/lazy-import-components/delayed-hydration/macro/index.vue(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.vue
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use
<script setup lang="ts">and the composition API when creating Vue components
Files:
test/fixtures/basic/app/pages/lazy-import-components/delayed-hydration/macro/index.vue
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Follow standard TypeScript conventions and best practices
Files:
packages/nuxt/src/components/module.tspackages/nuxt/test/lazy-hydration-macro-transform.test.tspackages/nuxt/src/components/plugins/lazy-hydration-macro-transform.ts
**/*.{test,spec}.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Write unit tests for core functionality using
vitest
Files:
packages/nuxt/test/lazy-hydration-macro-transform.test.ts
🧠 Learnings (3)
📚 Learning: 2024-12-12T12:36:34.871Z
Learnt from: huang-julien
PR: nuxt/nuxt#29366
File: packages/nuxt/src/app/components/nuxt-root.vue:16-19
Timestamp: 2024-12-12T12:36:34.871Z
Learning: In `packages/nuxt/src/app/components/nuxt-root.vue`, when optimizing bundle size by conditionally importing components based on route metadata, prefer using inline conditional imports like:
```js
const IsolatedPage = route?.meta?.isolate ? defineAsyncComponent(() => import('#build/isolated-page.mjs')) : null
```
instead of wrapping the import in a computed property or importing the component unconditionally.
Applied to files:
test/fixtures/basic/app/pages/lazy-import-components/delayed-hydration/macro/index.vuepackages/nuxt/test/lazy-hydration-macro-transform.test.ts
📚 Learning: 2025-07-18T16:46:07.446Z
Learnt from: CR
PR: nuxt/nuxt#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-18T16:46:07.446Z
Learning: Applies to **/*.vue : Use `<script setup lang="ts">` and the composition API when creating Vue components
Applied to files:
test/fixtures/basic/app/pages/lazy-import-components/delayed-hydration/macro/index.vue
📚 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/nuxt/src/components/plugins/lazy-hydration-macro-transform.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). (20)
- GitHub Check: test-fixtures (windows-latest, built, webpack, default, manifest-on, json, lts/-1)
- GitHub Check: test-fixtures (windows-latest, built, rspack, async, manifest-on, json, lts/-1)
- GitHub Check: test-fixtures (windows-latest, built, rspack, default, manifest-on, json, lts/-1)
- GitHub Check: test-fixtures (windows-latest, built, vite, async, manifest-on, json, lts/-1)
- GitHub Check: test-fixtures (windows-latest, dev, 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, webpack, async, manifest-on, json, lts/-1)
- GitHub Check: test-fixtures (windows-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, webpack, default, manifest-on, 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, default, 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, default, manifest-on, json, lts/-1)
- GitHub Check: test-fixtures (ubuntu-latest, built, vite, default, manifest-off, 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, async, manifest-on, json, lts/-1)
- GitHub Check: typecheck (ubuntu-latest, bundler)
- GitHub Check: typecheck (windows-latest, bundler)
- GitHub Check: test-benchmark
🔇 Additional comments (9)
packages/nuxt/src/components/plugins/lazy-hydration-macro-transform.ts (4)
7-8: LGTM! Good choice replacing regex with AST parsing.Using
oxc-walkerfor AST-based parsing is more robust than regex matching and properly handles edge cases like nested expressions and dynamic imports with.then()chains.
20-28: Well-structured strategy-to-factory mapping.The
HYDRATION_TO_FACTORYMap provides a clean mapping between hydration strategies and their corresponding runtime factory functions. This makes the transformation logic more maintainable.
61-109: Robust AST traversal with proper validation checks.The implementation correctly:
- Validates the call expression structure
- Ensures the macro is used within a variable declaration
- Extracts both direct imports and chained
.then()imports- Handles alias resolution for import paths
This addresses the concerns raised in the PR comments about handling complex import patterns.
137-145: Alias resolution implementation looks correct.The function properly:
- Iterates through aliases sorted by length (longest first)
- Handles path separators correctly
- Falls back to the original path if no alias matches
test/fixtures/basic/app/pages/lazy-import-components/delayed-hydration/macro/index.vue (1)
46-51: Import paths correctly updated to use alias syntax.The change from
~/app/components/to~/components/aligns with the new alias resolution support and demonstrates that the macro transform works with aliased paths.packages/nuxt/src/components/module.ts (1)
233-237: Correctly passes alias configuration to the transform plugin.Adding
alias: nuxt.options.aliasenables the LazyHydrationMacroTransformPlugin to resolve aliased import paths, which is essential for supporting components not discovered by Nuxt's auto-import.packages/nuxt/test/lazy-hydration-macro-transform.test.ts (3)
51-69: Good test coverage for non-auto-imported components.The new test case properly validates that lazy hydration macros work when component auto-import is disabled (the
noComponentsscenario), which directly addresses the PR's objective.
154-170: Transform helper correctly handles the noComponents flag.The updated function signature and conditional logic properly support testing both auto-imported and non-auto-imported component scenarios.
201-203: Alias configuration properly passed to the macro transform plugin.The test setup correctly configures the alias mapping to validate the alias resolution functionality.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/nuxt/src/components/plugins/lazy-hydration-macro-transform.ts (1)
133-136: String-literal guard misses oxcStringLiteral(and simple TemplateLiteral) variantsApply:
-function isStringLiteral (node: any): node is StringLiteral { - return !!node && node.type === 'Literal' && typeof node.value === 'string' -} +function isStringLiteral (node: any): node is StringLiteral { + if (!node) { return false } + // oxc variants + if ((node.type === 'Literal' || node.type === 'StringLiteral') && typeof node.value === 'string') { return true } + // handle `import(`~/comp`)` (no expressions) + if (node.type === 'TemplateLiteral' && Array.isArray(node.expressions) && node.expressions.length === 0) { + // oxc stores cooked string on quasis[0].value.cooked + const cooked = node.quasis?.[0]?.value?.cooked + if (typeof cooked === 'string') { (node as any).value = cooked; return true } + } + return false +}
🧹 Nitpick comments (5)
packages/nuxt/src/components/plugins/lazy-hydration-macro-transform.ts (5)
34-35: Minor: drop redundant default for required option
LoaderOptions.aliasis required;|| {}is unnecessary.Apply:
- const aliases = Object.entries(options.alias || {}).sort((a, b) => b[0].length - a[0].length) + const aliases = Object.entries(options.alias).sort((a, b) => b[0].length - a[0].length)
61-68: Macro detection is limited to variable declarationsCalls assigned via
AssignmentExpression(e.g.comp = defineLazyHydrationComponent(...)) or within export declarators won’t be transformed. Consider broadening parent guards or add a follow-up pass.Would you like a patch to support
AssignmentExpressionandExportNamedDeclarationinitialisers?
98-103: Accept template literals without expressions for import paths
import(~/x)(no interpolations) is common. Treat zero-expression TemplateLiterals as string-like.Combined with the
isStringLiteralupdate below, this will be handled automatically.
137-145: Alias resolution: add boundary check and keep query/hash intactPrevent false positives on prefixes (e.g.
#appvs#apply). Also ensure we don’t strip?query/#hashwhile resolving the path segment.Apply:
-function resolveAliases (aliases: [string, string][], filePath: string) { +function resolveAliases (aliases: [string, string][], filePath: string) { for (const [alias, target] of aliases) { - if (!filePath.startsWith(alias)) { continue } - const rest = filePath.slice(alias.length).replace(/^[/\\]+/, '') - return resolve(target, rest) + if (!filePath.startsWith(alias)) { continue } + const next = filePath.charAt(alias.length) + if (next && !['/', '\\'].includes(next)) { continue } + const [pathPart, suffix = ''] = filePath.slice(alias.length).split(/([?#].*)/, 2) // preserve ?/# + const rest = pathPart.replace(/^[/\\]+/, '') + return resolve(target, rest) + suffix } return filePath }
18-19: Regex prefilter is good; consider anchoring function name to avoid false hits in commentsLow risk, but you can require an opening parenthesis right after optional whitespace (already done) and ignore comments via AST anyway. Optional.
No change required if the extra handler guard is added.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/nuxt/src/components/plugins/lazy-hydration-macro-transform.ts(3 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/nuxt/src/components/plugins/lazy-hydration-macro-transform.ts
🧠 Learnings (2)
📓 Common learnings
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.
📚 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/nuxt/src/components/plugins/lazy-hydration-macro-transform.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). (3)
- GitHub Check: codeql (javascript-typescript)
- GitHub Check: build
- GitHub Check: code
🔇 Additional comments (5)
packages/nuxt/src/components/plugins/lazy-hydration-macro-transform.ts (5)
30-47: Plugin wiring and transformInclude look solidGood use of unplugin, late enforcement, and Nuxt-aware include/exclude gating. No issues here.
115-118: Import generationImport aggregation via
genImportwith aliasing is correct and minimal. Nice.
120-128: Sourcemaps and output handling look correctGood use of MagicString’s hires maps and only returning when changed.
20-28: All factories in HYDRATION_TO_FACTORY are exported by the runtime
createLazyVisibleComponent, createLazyIdleComponent, createLazyInteractionComponent, createLazyMediaQueryComponent, createLazyIfComponent, createLazyTimeComponent and createLazyNeverComponent are all exported in packages/nuxt/src/components/runtime/lazy-hydrated-component.ts (lines 31, 42, 57, 68, 78, 93, 109).
49-56: Skip adding a guard—unplugin supportstransform.filter.code.include, and the handler already exits early viaif (!matches.length) return. (unplugin.unjs.io, fossies.org)Likely an incorrect or invalid review comment.
packages/nuxt/src/components/plugins/lazy-hydration-macro-transform.ts
Outdated
Show resolved
Hide resolved
| const originalLoader = code.slice(loaderArgument.start, loaderArgument.end) | ||
| const replacement = `__${functionName}(${JSON.stringify(relativePath)}, ${originalLoader})` | ||
|
|
||
| const relativePath = relative(options.srcDir, component.filePath) | ||
| const dynamicImport = `${genDynamicImport(component.filePath, { interopDefault: false })}.then(c => c.${component.export ?? 'default'} || c)` | ||
| const replaceFunctionName = `createLazy${upperFirst(hydrationStrategy)}Component` | ||
| const replacement = `const ${variableName} = __${replaceFunctionName}(${JSON.stringify(relativePath)}, ${dynamicImport})` | ||
| edits.push({ start: node.start, end: node.end, replacement }) | ||
| names.add(functionName) | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
AST position fields: prefer span.start/end fallback to avoid oxc shape drift
Some oxc versions expose span.{start,end} instead of {start,end}. Slice positions defensively.
Apply:
- const originalLoader = code.slice(loaderArgument.start, loaderArgument.end)
+ const originalLoader = code.slice(getStart(loaderArgument), getEnd(loaderArgument))
const replacement = `__${functionName}(${JSON.stringify(relativePath)}, ${originalLoader})`
edits.push({ start: node.start, end: node.end, replacement })Add helpers:
function getStart (n: any) { return (n && (n.start ?? n.span?.start)) as number }
function getEnd (n: any) { return (n && (n.end ?? n.span?.end)) as number }And use them when pushing edits as well.
🤖 Prompt for AI Agents
In packages/nuxt/src/components/plugins/lazy-hydration-macro-transform.ts around
lines 104 to 109, the code uses node.start/node.end and
loaderArgument.start/loaderArgument.end directly but some oxc AST shapes expose
positions under span.start/span.end; add two helper functions getStart(n) and
getEnd(n) that return n.start ?? n.span?.start and n.end ?? n.span?.end
respectively, then replace all direct uses of .start/.end and slices so
edits.push uses getStart(node)/getEnd(node) and the originalLoader slice uses
getStart(loaderArgument)/getEnd(loaderArgument) to defensively handle both
shapes.
packages/nuxt/src/components/plugins/lazy-hydration-macro-transform.ts
Outdated
Show resolved
Hide resolved
danielroe
changed the title
danielroe
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/nuxt/src/components/plugins/lazy-hydration-macro-transform.ts (1)
115-117: Make string-literal guard resilient to oxc variantsSome oxc builds use StringLiteral. Accept both Literal and StringLiteral.
-function isStringLiteral (node: Argument | undefined) { - return !!node && node.type === 'Literal' && typeof node.value === 'string' -} +function isStringLiteral (node: any): node is { type: 'Literal' | 'StringLiteral', value: string } { + if (!node) { return false } + return ((node.type === 'Literal' || node.type === 'StringLiteral') && typeof node.value === 'string') +}
🧹 Nitpick comments (5)
packages/nuxt/src/components/plugins/lazy-hydration-macro-transform.ts (3)
68-76: Strategy resolution is correct; consider diagnostics on unknown strategiesCurrently unknown strategies are silently skipped. Optional: add a dev-only warn to aid debugging.
- const functionName = HYDRATION_TO_FACTORY.get(strategy) - if (!functionName) { return } + const functionName = HYDRATION_TO_FACTORY.get(strategy) + if (!functionName) { + if (process.env.NODE_ENV !== 'production') { + // eslint-disable-next-line no-console + console.warn(`[nuxt:lazy-hydration-macro] Unknown strategy "${strategy}" in ${id}`) + } + return + }
77-77: Broaden accepted loader shapes beyond ArrowFunctionExpressionMacro users may write function () { return import('...') }. Supporting FunctionExpression is a low-effort win.
- if (loaderArgument?.type !== 'ArrowFunctionExpression') { return } + if (loaderArgument?.type !== 'ArrowFunctionExpression' && loaderArgument?.type !== 'FunctionExpression') { return }And pass the correct body:
- const { importExpression, importLiteral } = findImportExpression(loaderArgument.body) + const body = loaderArgument.type === 'ArrowFunctionExpression' ? loaderArgument.body : loaderArgument.body as FunctionBody + const { importExpression, importLiteral } = findImportExpression(body)
86-91: Defensive positions: support oxc span.start/end to avoid shape driftAccessing .start/.end directly can break on oxc variants exposing span.{start,end}. Wrap with helpers and use them for slices and overwrites.
- const originalLoader = code.slice(loaderArgument.start, loaderArgument.end) + const originalLoader = code.slice(getStart(loaderArgument), getEnd(loaderArgument)) const replacement = `__${functionName}(${JSON.stringify(relativePath)}, ${originalLoader})` - edits.push({ start: node.start, end: node.end, replacement }) + edits.push({ start: getStart(node), end: getEnd(node), replacement })Add helpers (near the bottom of the file):
+function getStart (n: any) { return (n && (n.start ?? n.span?.start)) as number } +function getEnd (n: any) { return (n && (n.end ?? n.span?.end)) as number }Also applies to: 93-95
packages/nuxt/test/lazy-hydration-macro-transform.test.ts (2)
51-69: Add a case for named export resolution via .then(c => c.NamedExport)This ensures support for patterns mentioned in the PR comments.
+ it ('should support named exports returned from then()', async () => { + const sfc = ` + <script setup> + const Try = defineLazyHydrationComponent('interaction', () => import('~/components/MyComponent.vue').then(c => c.MyComponent)) + </script> + <template><Try /></template> + ` + const code = await transform(sfc, '/pages/index.vue') + expect(code).toContain(`import { createLazyInteractionComponent } from '../client-runtime.mjs';`) + const line = code.split('\n').map(l => l.trim()).find(l => l.startsWith('const Try')) + expect(line).toContain(`createLazyInteractionComponent("components/MyComponent.vue"`) + })
112-136: Minor: filename extension in TS testThe “ts” test currently uses index.tsx. Consider using .ts to reflect the test title.
- const code = await transform(component, '/pages/index.tsx') + const code = await transform(component, '/pages/index.ts')Also applies to: 138-140
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/nuxt/src/components/plugins/lazy-hydration-macro-transform.ts(3 hunks)packages/nuxt/test/lazy-hydration-macro-transform.test.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Follow standard TypeScript conventions and best practices
Files:
packages/nuxt/test/lazy-hydration-macro-transform.test.tspackages/nuxt/src/components/plugins/lazy-hydration-macro-transform.ts
**/*.{test,spec}.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Write unit tests for core functionality using
vitest
Files:
packages/nuxt/test/lazy-hydration-macro-transform.test.ts
🧠 Learnings (3)
📓 Common learnings
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.
📚 Learning: 2024-12-12T12:36:34.871Z
Learnt from: huang-julien
PR: nuxt/nuxt#29366
File: packages/nuxt/src/app/components/nuxt-root.vue:16-19
Timestamp: 2024-12-12T12:36:34.871Z
Learning: In `packages/nuxt/src/app/components/nuxt-root.vue`, when optimizing bundle size by conditionally importing components based on route metadata, prefer using inline conditional imports like:
```js
const IsolatedPage = route?.meta?.isolate ? defineAsyncComponent(() => import('#build/isolated-page.mjs')) : null
```
instead of wrapping the import in a computed property or importing the component unconditionally.
Applied to files:
packages/nuxt/test/lazy-hydration-macro-transform.test.tspackages/nuxt/src/components/plugins/lazy-hydration-macro-transform.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/nuxt/src/components/plugins/lazy-hydration-macro-transform.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 (10)
packages/nuxt/src/components/plugins/lazy-hydration-macro-transform.ts (7)
3-3: Good call: use Nuxt’s resolveAlias and thread alias through optionsImporting resolveAlias from pathe/utils and adding alias to LoaderOptions is the right direction and matches Nuxt conventions.
Also applies to: 16-16
21-29: Factory map is clear and future-proofMapping strategies to factory creators via HYDRATION_TO_FACTORY is explicit and easy to extend.
50-53: Cheap pre-filter is a solid optimisationUsing transform.filter.code.include with LAZY_HYDRATION_MACRO_RE avoids unnecessary AST walks.
60-67: Tight AST guards look goodRestricting to defineLazyHydrationComponent calls inside variable declarators reduces false positives.
79-85: Robust import() unwrapping and alias→relative resolution: LGTMHandling blocks/await/parentheses/conditionals/members/calls plus resolveAlias + pathe.relative matches real-world usages and fixes the “no auto-imports” mode.
97-101: Import injection via genImport is neatPrepending a single aggregated import for used factories keeps output tidy.
119-146: ImportExpression finder covers key shapesCovers blocks, await, parentheses, conditionals, member chains and calls. Nice.
packages/nuxt/test/lazy-hydration-macro-transform.test.ts (3)
41-48: Snapshot expectations align with alias→relative behaviourThe first-argument chunk key and retained loader look correct across SFC/JSX/TS.
Also applies to: 100-109, 142-150
153-177: Great coverage for tricky arrow/body/wrapping patternsBlock bodies, async/await, parentheses, member chains, conditionals, and nested combos are all exercised. Nice.
Also applies to: 179-200, 202-223, 225-246, 248-268, 270-288
291-306: Config scaffolding for no-components and alias looks goodnoComponents mode and alias: { '~/': '/' } accurately model the target scenario.
Also applies to: 335-341
Co-authored-by: Alex Liu <[email protected]>
🔗 Linked issue
resolves #33036
📚 Description
This PR should make sure that also component that are not "discovered" through nuxt (e.g. because
componentsis set tofalsein the config can be used in Lazy Hydration Macros)Related: #31192