perf(nuxt): enable Transition component only on client side
#30720
Conversation
|
|
@nuxt/kit
@nuxt/rspack-builder
nuxt
@nuxt/schema
@nuxt/vite-builder
@nuxt/webpack-builder
commit: |
CodSpeed Performance ReportMerging #30720 will not alter performanceComparing Summary
|
Pull Request Changes AnalysisWalkthroughThe pull request introduces significant changes to Nuxt's component rendering and transition handling. The modifications primarily focus on the The changes enhance the component rendering process, particularly for server-side rendering scenarios. A new approach to handling transitions has been implemented, with modifications to import statements and the way components are wrapped. The Additionally, a minor adjustment was made to a bundle size test, reflecting the impact of these architectural changes on the overall bundle size. ✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
![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 (3)
packages/nuxt/src/pages/runtime/utils.ts (1)
28-30: Consider adding type definitions for clarityThe
wrapInTransitionfunction currently usesanyfor thepropsandchildrenparameters. Defining explicit types for them would improve code reliability and maintainability, reducing uncertainty for future readers.-export const wrapInTransition = (props: any, children: any) => { +interface TransitionProps { + // Define specific transition props here (e.g. name?: string) +} + +interface TransitionChildren { + default?: () => VNode | VNode[] + // Add other slot definitions as needed +} + +export const wrapInTransition = (props: TransitionProps | boolean, children: TransitionChildren) => { return { default: () => import.meta.client && props ? h(Transition, props === true ? {} : props, children) : children.default?.() } }packages/nuxt/src/pages/runtime/page.ts (2)
106-123: Consider adding error boundaries for SSR<Suspense>The server-side
<Suspense>usage is sound. For resilience, handle error states or a fallback to ensure graceful failure in SSR.
158-158: Avoid mutating(providerVNode.type as any).nameAltering the VNode type's
nameproperty can cause confusion in devtools and may break assumptions about the component. Prefer naming patterns that do not rely on direct mutation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/nuxt/src/app/components/nuxt-layout.ts(2 hunks)packages/nuxt/src/app/components/utils.ts(1 hunks)packages/nuxt/src/pages/runtime/page.ts(4 hunks)packages/nuxt/src/pages/runtime/utils.ts(2 hunks)test/bundle.test.ts(1 hunks)
🔇 Additional comments (11)
packages/nuxt/src/app/components/nuxt-layout.ts (3)
2-2: Verify the necessity of importingSuspense
Suspenseis imported here. Confirm that it is indeed required in this file for the intended usage. If it is no longer needed, removing the import would lighten the code.
9-9: Replacement of_wrapIfwithwrapInTransitionThe switch to
wrapInTransitionaligns with the PR’s objective to enable<Transition>on the client side. The import is straightforward and appears correct.
83-83: Assess transition fallback logicWhen
hasLayout && transitionPropsis falsy, no transition is used. Confirm that omitting transitions in that scenario is the intended behaviour and does not adversely affect SSR or client rendering.packages/nuxt/src/app/components/utils.ts (1)
2-2: Unused import removedRemoving
Componentfrom the imports aligns with the removal of_wrapIf. This update helps keep the file lean and avoids importing unused types.test/bundle.test.ts (1)
130-130: Update to bundle size snapshotThe expected server bundle size changed from 303k to 301k. This suggests a slight reduction, which is beneficial. Keep the snapshot in sync if there are additional changes to the bundle size.
packages/nuxt/src/pages/runtime/page.ts (6)
1-1: No issues with updated importsThe newly added imports from 'vue' align well with the introduced logic.
7-7: Newly importedwrapInTransitionaligns with objectivesIntroducing
wrapInTransitionhere is consistent with the PR's goal of refining transition handling in the codebase.
103-103: Confirm concurrency handling forpreviousPageKeyAssigning
previousPageKey = keyhelps track route changes, but verify that no race conditions arise during concurrent or rapid navigations.
125-125: Client-side rendering commentThe comment adequately clarifies the client-side logic. No additional concerns.
Line range hint
126-135: Transition and keep-alive logic looks correctCombining merged transition properties with keep-alive ensures consistent behaviour between user-defined and default transitions.
Line range hint
136-157: Proper client-sidewrapInTransitionusageExecuting
wrapInTransitionandwrapInKeepAliveon the client side is a clear approach for conditionally running transitions. Nicely structured.
danielroe
changed the title
Transition component only on client sideTransition component only on client side
| previousPageKey = key | ||
|
|
||
| if (import.meta.server) { |
There was a problem hiding this comment.
looking at the vue implementation of Transition for SSR, is there any performance benefit or reason to have a separate implementation in nuxt? (I'm guessing yes, but just want to confirm.)
There was a problem hiding this comment.
I looked into the implementations of <KeepAlive> and <Transition>.
It seems that <KeepAlive> in SSR scenarios returns the default slots early on the server side without continuing to execute its internal implementation.
For <Transition>, I couldn’t find any related SSR-specific implementations in BaseTransition.
The reason for opening this PR is that <Transition> involves DOM operations, so even if the component is initialized on the server side, its functionality wouldn’t be utilized. Additionally, <Transition> does not generate extra DOM nodes, so skipping its execution on the server side will not cause hydration errors on the client side. Based on these considerations, I’ve made adjustments in this PR.
Separating the server-side and client-side implementations also helps to clearly show the differences between the two, rather than relying on numerous conditional checks to understand the logic.
If there are any adjustments or considerations I might have missed, please let me know. I truly appreciate your feedback. Thank you very much!
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/nuxt/src/pages/runtime/page.ts (2)
137-137: Refactor nested logic for readabilityConditionally wrapping everything with
_wrapInTransitionin the same statement could reduce readability over time. Consider extracting the keep-alive or transition arguments into local variables for cleaner code.
159-159: Avoid altering the component’s type name inlineModifying
(providerVNode.type as any).namecan confuse debugging and tool integrations. It may be clearer to pass a name via component options or refactor your approach to avoid direct reassignment.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/nuxt/src/app/components/nuxt-layout.ts(2 hunks)packages/nuxt/src/app/components/utils.ts(2 hunks)packages/nuxt/src/pages/runtime/page.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/nuxt/src/app/components/nuxt-layout.ts
🔇 Additional comments (5)
packages/nuxt/src/pages/runtime/page.ts (3)
1-1: No concerns with the updated importsThese newly added imports are appropriate for advanced component rendering in Vue and fully align with the new SSR approach.
12-12: Consistent switch to_wrapInTransitionReplacing
_wrapIfwith_wrapInTransitionat import time is coherent with the new transition usage. Please ensure any remaining references to_wrapIfare removed throughout the codebase.
107-125: Consider adding SSR fallback testsYou have introduced a Suspense-based approach for server-side rendering. While this looks correct, please ensure that fallback scenarios for errors and timeouts are thoroughly tested to avoid hydration issues.
packages/nuxt/src/app/components/utils.ts (2)
1-2: Imports look goodThese new imports for
Transition,createStaticVNode, andhmatch their usage in this module. No compatibility concerns observed.
13-13: Handle edge cases in_wrapInTransitionThis new function correctly ensures transitions only run on the client. However, verify that when
children.defaultis undefined or false, the code gracefully returns without error.
🔗 Linked issue
📚 Description
wrapInKeepAlive, ensuring that the<Transition>components inside<NuxtPage>and<NuxtLayout>are initialized only on the client side.