fix(lazy-load): prefer cloning component over wrapping component#232
Merged
huang-julien merged 5 commits intomainfrom Feb 22, 2026
Merged
fix(lazy-load): prefer cloning component over wrapping component#232huang-julien merged 5 commits intomainfrom
huang-julien merged 5 commits intomainfrom
Conversation
commit: |
📝 WalkthroughWalkthroughImports in the lazy-load composables were adjusted to use a type-only import from Vue. Two helpers, __wrapMainComponent and __wrapImportedComponent, received documentation and now augment components' setup functions in-place: __wrapMainComponent calls useLazyComponentTracking(imports) before invoking the original setup; __wrapImportedComponent tracks usage, ensures state.directImports entries exist and marks components as rendered before invoking the original setup (or returning undefined if none). No public API signatures changed. A new comprehensive test suite for these composables was added and the runtime test glob was broadened to include nested runtime tests. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/runtime/lazy-load/composables.ts (2)
1-1:⚠️ Potential issue | 🟡 MinorRemove unused
defineComponentandhimports — CI failure.Both
defineComponentandhwere only used in the old wrapper-component approach. Neither is referenced anywhere in the file after this change.🔧 Proposed fix
-import { defineComponent, h, type DefineComponent } from 'vue' +import { type DefineComponent } from 'vue'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/runtime/lazy-load/composables.ts` at line 1, The file imports unused symbols defineComponent and h which cause CI failures; update the import statement in src/runtime/lazy-load/composables.ts to remove defineComponent and h and only keep the used symbol(s) (e.g., type DefineComponent) so the import becomes minimal and the linter/CI will pass.
55-77:⚠️ Potential issue | 🟠 MajorIn-place mutation of
component.setupon a shared module singleton risks double-wrapping.Vue's
defineComponentreturns the same object at runtime. In ESM, the component definition is a module-level singleton, so if two parent SFCs each call__wrapImportedComponenton the sameChildComp:
- Second call captures the already-wrapped setup as
originalSetup, nesting the setup chain.- Each render invokes
useLazyComponentTracking()twice.- The
importedByregistered indirectImportsreflects whichever parent's module was evaluated last — non-deterministic.The PR title says "prefer cloning" but the implementation mutates in place. Shallow-cloning the component before patching
setupboth prevents the double-wrap and matches the stated intent:🛠️ Proposed fix — clone before patching
- const originalSetup = component.setup - - component.setup = (props, ctx) => { + const cloned: DefineComponent = { ...component } + const originalSetup = cloned.setup + + cloned.setup = (props, ctx) => { const state = useLazyComponentTracking() if (state) { if (!state.directImports.has(componentName)) { state.directImports.set(componentName, { componentName, importSource, importedBy, rendered: false, }) } const info = state.directImports.get(componentName) if (info) { info.rendered = true } } return originalSetup ? originalSetup(props, ctx) : undefined } - return component + return cloned🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/runtime/lazy-load/composables.ts` around lines 55 - 77, The current code mutates component.setup in place which can double-wrap due to module singletons; instead, shallow-clone the component object before patching so each call to __wrapImportedComponent operates on a fresh object. Create a shallow copy of the incoming component (copy properties like setup, name, etc.), capture originalSetup from the clone, then assign the wrapped setup on the clone (leaving the original module export untouched), and return the cloned component; keep using useLazyComponentTracking(), componentName, importSource and importedBy as before so directImports stays consistent while avoiding nested setups and non-deterministic importedBy updates.
🧹 Nitpick comments (2)
test/runtime/lazy-load/composables.test.ts (1)
116-184: Well-structured coverage overall; consider adding a double-wrap test.The existing cases are correct and thorough. If the composables fix adopts the shallow-clone approach (flagged in
composables.ts), a regression test guarding that the original component'ssetupis not mutated and that a second call for a differentimportedByreturns an independent clone would be valuable:it('should not mutate the original component when wrapping', () => { const comp = makeComponent() const originalSetup = comp.setup __wrapImportedComponent(comp, 'MyComp', './MyComp.vue', 'ParentA.vue') // Original component's setup must remain unchanged expect(comp.setup).toBe(originalSetup) }) it('should produce independent clones for different importedBy callers', () => { const comp = makeComponent() const wrappedA = __wrapImportedComponent(comp, 'MyComp', './MyComp.vue', 'ParentA.vue') const wrappedB = __wrapImportedComponent(comp, 'MyComp', './MyComp.vue', 'ParentB.vue') expect(wrappedA).not.toBe(wrappedB) expect(wrappedA.setup).not.toBe(wrappedB.setup) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/runtime/lazy-load/composables.test.ts` around lines 116 - 184, Add two tests to ensure __wrapImportedComponent does not mutate the original component and returns independent clones per importer: 1) verify that calling __wrapImportedComponent(comp, 'MyComp', './MyComp.vue', 'ParentA.vue') leaves comp.setup unchanged (compare reference to originalSetup), and 2) call __wrapImportedComponent twice with different importedBy values (e.g., 'ParentA.vue' and 'ParentB.vue') and assert the returned components are not the same object and their setup functions are different references; reference the __wrapImportedComponent function and the component produced by makeComponent in your assertions.src/runtime/lazy-load/composables.ts (1)
57-75: Two optional cleanups in thesetupoverride.
- Redundant
if (state)guard (line 59) —useLazyComponentTracking()always initialises and returns the state object; it never returns a falsy value, so this branch can never be false.- Inconsistent indentation — lines 58–73 are indented by 6 spaces while line 74 uses 4 spaces.
♻️ Proposed cleanup
component.setup = (props, ctx) => { - const state = useLazyComponentTracking() - if (state) { - if (!state.directImports.has(componentName)) { - state.directImports.set(componentName, { - componentName, - importSource, - importedBy, - rendered: false, - }) - } - - const info = state.directImports.get(componentName) - if (info) { - info.rendered = true - } - } - return originalSetup ? originalSetup(props, ctx) : undefined - } + const state = useLazyComponentTracking() + if (!state.directImports.has(componentName)) { + state.directImports.set(componentName, { + componentName, + importSource, + importedBy, + rendered: false, + }) + } + const info = state.directImports.get(componentName) + if (info) { + info.rendered = true + } + return originalSetup ? originalSetup(props, ctx) : undefined + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/runtime/lazy-load/composables.ts` around lines 57 - 75, Remove the unnecessary runtime guard around the setup override: call useLazyComponentTracking() and operate on the returned state directly (delete the "if (state) { ... }" block) because useLazyComponentTracking() always returns a valid object; keep the logic that checks and sets state.directImports (componentName, importSource, importedBy, rendered) and update info.rendered, then return originalSetup(props, ctx) as before. Also normalize indentation in the component.setup override so all lines inside the function use consistent spacing. Ensure you reference the existing symbols component.setup, useLazyComponentTracking, state.directImports, componentName, importSource, importedBy, info, and originalSetup when making the edits.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/runtime/lazy-load/composables.ts`:
- Line 1: The file imports unused symbols defineComponent and h which cause CI
failures; update the import statement in src/runtime/lazy-load/composables.ts to
remove defineComponent and h and only keep the used symbol(s) (e.g., type
DefineComponent) so the import becomes minimal and the linter/CI will pass.
- Around line 55-77: The current code mutates component.setup in place which can
double-wrap due to module singletons; instead, shallow-clone the component
object before patching so each call to __wrapImportedComponent operates on a
fresh object. Create a shallow copy of the incoming component (copy properties
like setup, name, etc.), capture originalSetup from the clone, then assign the
wrapped setup on the clone (leaving the original module export untouched), and
return the cloned component; keep using useLazyComponentTracking(),
componentName, importSource and importedBy as before so directImports stays
consistent while avoiding nested setups and non-deterministic importedBy
updates.
---
Nitpick comments:
In `@src/runtime/lazy-load/composables.ts`:
- Around line 57-75: Remove the unnecessary runtime guard around the setup
override: call useLazyComponentTracking() and operate on the returned state
directly (delete the "if (state) { ... }" block) because
useLazyComponentTracking() always returns a valid object; keep the logic that
checks and sets state.directImports (componentName, importSource, importedBy,
rendered) and update info.rendered, then return originalSetup(props, ctx) as
before. Also normalize indentation in the component.setup override so all lines
inside the function use consistent spacing. Ensure you reference the existing
symbols component.setup, useLazyComponentTracking, state.directImports,
componentName, importSource, importedBy, info, and originalSetup when making the
edits.
In `@test/runtime/lazy-load/composables.test.ts`:
- Around line 116-184: Add two tests to ensure __wrapImportedComponent does not
mutate the original component and returns independent clones per importer: 1)
verify that calling __wrapImportedComponent(comp, 'MyComp', './MyComp.vue',
'ParentA.vue') leaves comp.setup unchanged (compare reference to originalSetup),
and 2) call __wrapImportedComponent twice with different importedBy values
(e.g., 'ParentA.vue' and 'ParentB.vue') and assert the returned components are
not the same object and their setup functions are different references;
reference the __wrapImportedComponent function and the component produced by
makeComponent in your assertions.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/runtime/lazy-load/composables.ts (1)
1-1:⚠️ Potential issue | 🟠 MajorRemove unused
defineComponentandhimports — CI is failing.Neither
defineComponentnorhis referenced anywhere in this file's implementation. The pipeline confirmsdefineComponenttriggers a lint error;his equally unused.🔧 Proposed fix
-import { defineComponent, h, type DefineComponent } from 'vue' +import type { DefineComponent } from 'vue'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/runtime/lazy-load/composables.ts` at line 1, The import statement in composables.ts unnecessarily includes defineComponent and h which are not used and cause CI/lint failures; update the import to only import the needed symbol(s) (keep type DefineComponent) by removing defineComponent and h from the import list (or split runtime vs type imports so only used identifiers remain).
🧹 Nitpick comments (4)
test/runtime/lazy-load/composables.test.ts (2)
81-114: Consider adding a double-wrap guard test.Neither
__wrapMainComponentnor__wrapImportedComponentis tested when called twice on the same component object. Given the in-place mutation design, a second call would stackuseLazyComponentTrackingcalls (captured inoriginalSetup). A test asserting idempotent behaviour (or documenting the expected stacking) would prevent accidental regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/runtime/lazy-load/composables.test.ts` around lines 81 - 114, Add a test that calls __wrapMainComponent (and similarly __wrapImportedComponent) twice on the same component and asserts idempotent behavior: call __wrapMainComponent(comp, imports) twice, invoke wrapped.setup and ensure originalSetup/useLazyComponentTracking is not stacked (e.g. the returned setup value is the same as single-wrap case and the nuxtApp.payload._lazyHydrationState.directImports only contains one entry for the component), and include a similar case for components without an original setup to ensure repeated wrapping returns undefined; reference __wrapMainComponent, __wrapImportedComponent, originalSetup and useLazyComponentTracking in the test to locate the wrapping logic.
90-90: Replace repeatedas anycasts with a shared typed mock — CI is failing.The same inline context mock with
as anyappears ~8 times. Extracting it as a typed constant fixes the lint failures and removes duplication.♻️ Proposed fix — add a shared `mockCtx` at the top of the test file
+const mockCtx = { attrs: {}, slots: {}, emit: () => {}, expose: () => {} } as unknown as SetupContext // Then replace every occurrence, e.g.: -wrapped.setup!({}, { attrs: {}, slots: {}, emit: () => {}, expose: () => {} } as any) +wrapped.setup!({}, mockCtx)Also applies to: 103-103, 111-111, 121-121, 142-142, 164-164, 173-173, 181-181
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/runtime/lazy-load/composables.test.ts` at line 90, Create a shared typed mock context constant (e.g. const mockCtx = { attrs: {}, slots: {}, emit: () => {}, expose: () => {} } as any) at the top of the test file and replace every inline cast instance like wrapped.setup!({}, { attrs: {}, slots: {}, emit: () => {}, expose: () => {} } as any) with wrapped.setup!({}, mockCtx); update all calls referencing the inline mock (the occurrences around wrapped.setup!) so the repeated `as any` casts are removed and the single `mockCtx` is reused.src/runtime/lazy-load/composables.ts (2)
27-38: In-place mutation doesn't match the PR's stated "clone" semantics — consider a shallow copy.Both helpers directly assign
component.setup, permanently mutating the shared component definition object. If either helper is called more than once for the same component reference (HMR, test reuse, future refactors),originalSetupcaptures the already-augmented setup anduseLazyComponentTrackingfires multiple times per mount. A shallow clone isolates the mutation:♻️ Proposed fix for `__wrapMainComponent`
export function __wrapMainComponent( component: DefineComponent, imports: DirectImportInfo[] = [], ): DefineComponent { + component = { ...component } const originalSetup = component.setup component.setup = (props, ctx) => { useLazyComponentTracking(imports) return originalSetup ? originalSetup(props, ctx) : undefined } return component }Apply the same pattern to
__wrapImportedComponent(Line 55 onwards):♻️ Proposed fix for `__wrapImportedComponent`
const originalSetup = component.setup + component = { ...component } component.setup = (props, ctx) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/runtime/lazy-load/composables.ts` around lines 27 - 38, The helpers mutate the incoming component by reassigning component.setup which causes double-wrapping on repeated calls; instead make a shallow clone of the component object, capture the original setup from the clone (e.g. const wrapped = { ...component } as DefineComponent; const originalSetup = wrapped.setup), assign wrapped.setup = (props, ctx) => { useLazyComponentTracking(imports); return originalSetup ? originalSetup(props, ctx) : undefined }, and return the cloned wrapped component; apply the same shallow-clone-and-wrap approach to __wrapImportedComponent so you never mutate the original component reference and avoid multiple tracking invocations.
58-73:if (state)is always truthy — optional cleanup.
useLazyComponentTracking()always initialises and returnsstate; the null-guard at Line 59 is never falsy. Removing it reduces indentation and makes the invariant explicit.♻️ Proposed simplification
component.setup = (props, ctx) => { const state = useLazyComponentTracking() - if (state) { - if (!state.directImports.has(componentName)) { - state.directImports.set(componentName, { - componentName, - importSource, - importedBy, - rendered: false, - }) - } - - const info = state.directImports.get(componentName) - if (info) { - info.rendered = true - } + if (!state.directImports.has(componentName)) { + state.directImports.set(componentName, { + componentName, + importSource, + importedBy, + rendered: false, + }) } + const info = state.directImports.get(componentName) + if (info) { + info.rendered = true + } return originalSetup ? originalSetup(props, ctx) : undefined }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/runtime/lazy-load/composables.ts` around lines 58 - 73, The null-guard around useLazyComponentTracking() is redundant because useLazyComponentTracking always returns an initialized state; remove the if (state) check and operate on state directly: call const state = useLazyComponentTracking(); then check/insert into state.directImports using componentName/importSource/importedBy and set info.rendered = true—optionally add a brief comment or a non-null assertion to make the invariant explicit around state and state.directImports (referencing useLazyComponentTracking, state, directImports, componentName, importSource, importedBy, and info).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/runtime/lazy-load/composables.test.ts`:
- Line 108: There are trailing spaces after the line that defines comp using
defineComponent (the line containing "const comp = defineComponent({ render: ()
=> h('div') }) as unknown as DefineComponent"); remove the trailing whitespace
at the end of that line so the file contains no trailing spaces (ensure the
defineComponent line ends immediately after the semicolon) to satisfy CI.
- Line 9: The import line in composables.test.ts includes unused symbols
Component and DefineSetupFnComponent; remove Component and
DefineSetupFnComponent from the import list (keep defineComponent, h,
DefineComponent, SetupContext) so the import becomes only the actually used
symbols and the linter/CI will stop failing; update the import statement that
declares defineComponent, h, type Component, type DefineComponent, type
DefineSetupFnComponent, type SetupContext by deleting the unused type Component
and type DefineSetupFnComponent entries.
---
Outside diff comments:
In `@src/runtime/lazy-load/composables.ts`:
- Line 1: The import statement in composables.ts unnecessarily includes
defineComponent and h which are not used and cause CI/lint failures; update the
import to only import the needed symbol(s) (keep type DefineComponent) by
removing defineComponent and h from the import list (or split runtime vs type
imports so only used identifiers remain).
---
Nitpick comments:
In `@src/runtime/lazy-load/composables.ts`:
- Around line 27-38: The helpers mutate the incoming component by reassigning
component.setup which causes double-wrapping on repeated calls; instead make a
shallow clone of the component object, capture the original setup from the clone
(e.g. const wrapped = { ...component } as DefineComponent; const originalSetup =
wrapped.setup), assign wrapped.setup = (props, ctx) => {
useLazyComponentTracking(imports); return originalSetup ? originalSetup(props,
ctx) : undefined }, and return the cloned wrapped component; apply the same
shallow-clone-and-wrap approach to __wrapImportedComponent so you never mutate
the original component reference and avoid multiple tracking invocations.
- Around line 58-73: The null-guard around useLazyComponentTracking() is
redundant because useLazyComponentTracking always returns an initialized state;
remove the if (state) check and operate on state directly: call const state =
useLazyComponentTracking(); then check/insert into state.directImports using
componentName/importSource/importedBy and set info.rendered = true—optionally
add a brief comment or a non-null assertion to make the invariant explicit
around state and state.directImports (referencing useLazyComponentTracking,
state, directImports, componentName, importSource, importedBy, and info).
In `@test/runtime/lazy-load/composables.test.ts`:
- Around line 81-114: Add a test that calls __wrapMainComponent (and similarly
__wrapImportedComponent) twice on the same component and asserts idempotent
behavior: call __wrapMainComponent(comp, imports) twice, invoke wrapped.setup
and ensure originalSetup/useLazyComponentTracking is not stacked (e.g. the
returned setup value is the same as single-wrap case and the
nuxtApp.payload._lazyHydrationState.directImports only contains one entry for
the component), and include a similar case for components without an original
setup to ensure repeated wrapping returns undefined; reference
__wrapMainComponent, __wrapImportedComponent, originalSetup and
useLazyComponentTracking in the test to locate the wrapping logic.
- Line 90: Create a shared typed mock context constant (e.g. const mockCtx = {
attrs: {}, slots: {}, emit: () => {}, expose: () => {} } as any) at the top of
the test file and replace every inline cast instance like wrapped.setup!({}, {
attrs: {}, slots: {}, emit: () => {}, expose: () => {} } as any) with
wrapped.setup!({}, mockCtx); update all calls referencing the inline mock (the
occurrences around wrapped.setup!) so the repeated `as any` casts are removed
and the single `mockCtx` is reused.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/runtime/lazy-load/composables.test.ts`:
- Line 90: The tests use unsafe inline casts to any for SetupContext when
calling wrapped.setup — create a single typed mock context constant (e.g., const
mockSetupContext: SetupContext = { attrs: {}, slots: {}, emit: () => {}, expose:
() => {} }) after makeComponent and reuse it instead of casting to any; then
replace the inline casts at wrapped.setup! calls (lines referencing
wrapped.setup) across the file (including the instances at lines 90, 103, 111,
121, 142, 164, 181) to pass mockSetupContext so lint errors are removed and
typing is preserved.
---
Duplicate comments:
In `@test/runtime/lazy-load/composables.test.ts`:
- Line 9: The import line imports unused symbols Component and
DefineSetupFnComponent which are flagged by the linter; remove Component and
DefineSetupFnComponent from the import list while keeping the existing used
imports (e.g., defineComponent, h, DefineComponent, SetupContext) so the file
(composables.test.ts) no longer imports the unused identifiers and CI/linting
will pass.
|
|
||
| const wrapped = __wrapMainComponent(comp, imports) | ||
| // Invoke setup to trigger registration | ||
| wrapped.setup!({}, { attrs: {}, slots: {}, emit: () => {}, expose: () => {} } as any) |
There was a problem hiding this comment.
Replace as any with a typed mock context — CI is failing on all 6 reported sites (and line 181 follows the same pattern).
SetupContext is already imported. Define a shared constant once and reuse it, eliminating both the any lint errors and the verbosity.
🔧 Proposed fix — add a shared mock context constant and apply it everywhere
Add after line 16 (after makeComponent):
+const mockCtx = {
+ attrs: {},
+ slots: {},
+ emit: () => {},
+ expose: () => {},
+} as unknown as SetupContextThen replace every inline cast across the file:
-wrapped.setup!({}, { attrs: {}, slots: {}, emit: () => {}, expose: () => {} } as any)
+wrapped.setup!({}, mockCtx)-const result = wrapped.setup!({}, { attrs: {}, slots: {}, emit: () => {}, expose: () => {} } as any)
+const result = wrapped.setup!({}, mockCtx)Apply the same substitution on lines 103, 111, 121, 142, 164, and 181.
Also applies to: 103-103, 111-111, 121-121, 142-142, 164-164, 181-181
🧰 Tools
🪛 GitHub Check: ci
[failure] 90-90:
Unexpected any. Specify a different type
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/runtime/lazy-load/composables.test.ts` at line 90, The tests use unsafe
inline casts to any for SetupContext when calling wrapped.setup — create a
single typed mock context constant (e.g., const mockSetupContext: SetupContext =
{ attrs: {}, slots: {}, emit: () => {}, expose: () => {} }) after makeComponent
and reuse it instead of casting to any; then replace the inline casts at
wrapped.setup! calls (lines referencing wrapped.setup) across the file
(including the instances at lines 90, 103, 111, 121, 142, 164, 181) to pass
mockSetupContext so lint errors are removed and typing is preserved.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/runtime/lazy-load/composables.ts (1)
33-36:⚠️ Potential issue | 🟠 MajorAdd idempotency guards to prevent double-wrapping of shared components.
Both
__wrapMainComponentand__wrapImportedComponentmutatecomponent.setupdirectly without checking if wrapping has already been applied. When a shared component (e.g.,Button.vue) is imported by multiple pages, Vite's module system creates a single component singleton. The build transform generates separate wrapper calls for each importing file at module scope—ifPageA.vueandPageB.vueboth import Button, Button's setup gets wrapped twice, creating a chain. The first wrapper already calls the original setup, so the second wrapper wraps the wrapped version, causing:
- Redundant
useLazyComponentTracking()calls on every render- Incorrect
importedByattribution (always from the last-loaded page)Add a module-level
WeakSetguard to both functions:🛡️ Proposed fix — WeakSet guard
+const _wrappedComponents = new WeakSet<object>() export function __wrapMainComponent( component: DefineComponent, imports: DirectImportInfo[] = [], ): DefineComponent { + if (_wrappedComponents.has(component)) { + return component + } + _wrappedComponents.add(component) + const originalSetup = component.setup ... } export function __wrapImportedComponent( component: DefineComponent, componentName: string, importSource: string, importedBy: string, ) { if (component && component.name === 'AsyncComponentWrapper') { return component } + if (_wrappedComponents.has(component)) { + return component + } + _wrappedComponents.add(component) + const originalSetup = component.setup ... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/runtime/lazy-load/composables.ts` around lines 33 - 36, The component wrapping functions __wrapMainComponent and __wrapImportedComponent currently mutate component.setup without guarding against repeated wrapping; add a module-level WeakSet (e.g., wrappedComponents) and have both functions check wrappedComponents.has(component) before mutating and return early if true, otherwise perform the existing wrap (calling useLazyComponentTracking(imports) and delegating to originalSetup) and then add the component to wrappedComponents to ensure idempotency and avoid double-wrapping and duplicated tracking/importedBy attribution.
🧹 Nitpick comments (2)
test/runtime/lazy-load/composables.test.ts (1)
19-23:beforeEachonly resets the payload state, not component object mutations.This is fine because every test creates a new
compviamakeComponent()or an inlinedefineComponent(...). If a future test were to hoist a component to thedescribescope and reuse it across tests, the stacked setup wrappers from__wrapImportedComponent/__wrapMainComponentwould carry over. Worth a brief comment to warn future contributors.📝 Suggested comment
beforeEach(() => { const nuxtApp = useNuxtApp() // Reset lazy hydration state between tests delete nuxtApp.payload._lazyHydrationState + // Note: __wrapMainComponent and __wrapImportedComponent mutate component.setup + // in-place; always create a fresh component instance per test to avoid stacking. })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/runtime/lazy-load/composables.test.ts` around lines 19 - 23, The beforeEach currently only clears nuxtApp.payload._lazyHydrationState but doesn't undo mutations applied to component objects by the wrapper utilities; add a concise inline comment above the beforeEach explaining that tests create fresh components via makeComponent() or defineComponent(...) so component wrappers (__wrapImportedComponent and __wrapMainComponent) won't leak, and warn future contributors that if they hoist a component into describe scope they must either recreate/reset that component between tests or explicitly remove/restore wrappers (or call a provided cleanup) to avoid stacked setup wrappers carrying over across tests.src/runtime/lazy-load/composables.ts (1)
57-75:if (state)is always truthy; consider simplifying the double Map lookup.
useLazyComponentTracking()always initialises_lazyHydrationStatebefore returning it — it can never return a falsy value. Theif (state)guard on line 59 is misleading (implies state could be absent) and adds unnecessary nesting.Additionally, the
has(componentName)+get(componentName)pattern on lines 60–71 is two lookups for the same key.♻️ Proposed simplification
component.setup = (props, ctx) => { - const state = useLazyComponentTracking() - if (state) { - if (!state.directImports.has(componentName)) { - state.directImports.set(componentName, { - componentName, - importSource, - importedBy, - rendered: false, - }) - } - - const info = state.directImports.get(componentName) - if (info) { - info.rendered = true - } - } - return originalSetup ? originalSetup(props, ctx) : undefined + const state = useLazyComponentTracking() + const info = state.directImports.get(componentName) ?? { + componentName, + importSource, + importedBy, + rendered: false, + } + state.directImports.set(componentName, info) + info.rendered = true + return originalSetup ? originalSetup(props, ctx) : undefined }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/runtime/lazy-load/composables.ts` around lines 57 - 75, The code currently checks if (state) even though useLazyComponentTracking() always returns a valid state and does a has(componentName) followed by get(componentName) causing two Map lookups; remove the unnecessary if (state) guard and simplify the directImports handling by doing a single lookup: let info = state.directImports.get(componentName); if (!info) { info = { componentName, importSource, importedBy, rendered: false }; state.directImports.set(componentName, info); } then set info.rendered = true and finally return originalSetup ? originalSetup(props, ctx) : undefined — update the logic inside component.setup accordingly (referencing component.setup, useLazyComponentTracking, state.directImports, componentName, importSource, importedBy, originalSetup).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/runtime/lazy-load/composables.ts`:
- Around line 33-36: The component wrapping functions __wrapMainComponent and
__wrapImportedComponent currently mutate component.setup without guarding
against repeated wrapping; add a module-level WeakSet (e.g., wrappedComponents)
and have both functions check wrappedComponents.has(component) before mutating
and return early if true, otherwise perform the existing wrap (calling
useLazyComponentTracking(imports) and delegating to originalSetup) and then add
the component to wrappedComponents to ensure idempotency and avoid
double-wrapping and duplicated tracking/importedBy attribution.
---
Nitpick comments:
In `@src/runtime/lazy-load/composables.ts`:
- Around line 57-75: The code currently checks if (state) even though
useLazyComponentTracking() always returns a valid state and does a
has(componentName) followed by get(componentName) causing two Map lookups;
remove the unnecessary if (state) guard and simplify the directImports handling
by doing a single lookup: let info = state.directImports.get(componentName); if
(!info) { info = { componentName, importSource, importedBy, rendered: false };
state.directImports.set(componentName, info); } then set info.rendered = true
and finally return originalSetup ? originalSetup(props, ctx) : undefined —
update the logic inside component.setup accordingly (referencing
component.setup, useLazyComponentTracking, state.directImports, componentName,
importSource, importedBy, originalSetup).
In `@test/runtime/lazy-load/composables.test.ts`:
- Around line 19-23: The beforeEach currently only clears
nuxtApp.payload._lazyHydrationState but doesn't undo mutations applied to
component objects by the wrapper utilities; add a concise inline comment above
the beforeEach explaining that tests create fresh components via makeComponent()
or defineComponent(...) so component wrappers (__wrapImportedComponent and
__wrapMainComponent) won't leak, and warn future contributors that if they hoist
a component into describe scope they must either recreate/reset that component
between tests or explicitly remove/restore wrappers (or call a provided cleanup)
to avoid stacked setup wrappers carrying over across tests.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🔗 Linked issue
fix #231
❓ Type of change
📚 Description
This PR makes __wrapImportedComponent clone the component instead of using a wrapper component