refactor(nuxt): define layouts as async vue components#29957
Conversation
|
|
WalkthroughThe pull request modifies the Changes
Possibly related PRs
Suggested labels
Poem
Warning Rate limit exceeded@danielroe has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 12 minutes and 4 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 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 (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/nuxt/src/app/components/nuxt-layout.ts (1)
26-29: LGTM! Performance improvement through sync component loading.The change from async to sync layout loading is a good optimization. By moving the async handling to build time, we reduce runtime overhead and improve performance, which aligns with the PR objectives.
This architectural change follows Vue.js best practices by pre-compiling components and avoiding unnecessary async operations at runtime.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/nuxt/src/app/components/nuxt-layout.ts(1 hunks)packages/nuxt/src/core/templates.ts(1 hunks)
🔇 Additional comments (2)
packages/nuxt/src/app/components/nuxt-layout.ts (1)
26-29: Verify build-time layout compilation.
Since we're now assuming synchronous access to layouts, we should verify that all layouts are properly pre-compiled during the build phase.
✅ Verification successful
Based on the examination of the codebase, I can now generate a final response regarding the layout compilation verification.
Layout compilation is properly handled at build time.
The codebase shows that layouts are:
- Imported synchronously from a virtual module
#build/layouts(as seen in nuxt-layout.ts) - Registered during app initialization via the
addLayoutfunction inpackages/kit/src/layout.ts - Compiled and added to the app templates through the
app:templateshook - Accessed synchronously in the
LayoutLoadercomponent without any dynamic imports
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that layouts are properly compiled as Vue components
# Check the layout template generation
ast-grep --pattern 'export const layoutTemplate = $layout'
# Check for any remaining async imports in layouts
rg -l "import.*async.*from.*layouts"
# Look for any dynamic import() calls for layouts
rg "import\(.*layout.*\)"
Length of output: 127
Script:
#!/bin/bash
# Let's try a different approach to verify layout compilation
# Check how layouts are registered/handled in the codebase
rg -A 5 "layouts\s*=\s*{"
# Look for layout-related build/compilation logic
rg -l "layout.*build|build.*layout"
# Check for any layout registration or compilation utilities
ast-grep --pattern 'function $name($params) {
$$$
layout
$$$
}'
# Look for layout-related plugins or modules
fd "layout" --type f
Length of output: 1148
Script:
#!/bin/bash
# Let's examine the layout compilation and registration process
# Check the content of layout-related files
cat packages/kit/src/layout.ts
# Look for layout registration in nuxt-layout component
cat packages/nuxt/src/app/components/nuxt-layout.ts
# Check the layout plugin
cat packages/nuxt/src/app/plugins/check-if-layout-used.ts
# Search for layout compilation/build references with more context
rg -A 5 "defineNuxtConfig|buildModule.*layout"
Length of output: 77379
packages/nuxt/src/core/templates.ts (1)
288-288: LGTM! Well-structured implementation of async Vue components
The changes properly implement async Vue components by:
- Importing
defineAsyncComponentfrom Vue - Wrapping layout components with
defineAsyncComponent
This refactor aligns with Vue 3 best practices and should provide better type safety and runtime performance.
Also applies to: 291-291
| const LayoutComponent = await layouts[props.name]().then((r: any) => r.default || r) | ||
|
|
||
| return () => h(LayoutComponent, props.layoutProps, context.slots) | ||
| return () => h(layouts[props.name], props.layoutProps, context.slots) |
There was a problem hiding this comment.
@danielroe is there any chance that the removal of await before calling layouts[props.name] here (or another change in this PR) could cause a hydration error in some scenarios?
After upgrading to v3.14.1592 (the only change in my project) I'm now seeing Hydration completed but contains mismatches on a page that only contains a NuxtLayout and a paragraph tag in the default slot.
It doesn't occur when the dev server is running; only when building and running the build (e.g. with wrangler).
I've tried putting together a reproduction but (of course) it doesn't happen there as of yet.
Do you have any suggestions on how to further triage? Typically I'd start disabling modules, etc. but in a larger project, that isn't the easiest task since so many things depend on others 😬
There was a problem hiding this comment.
It actually also seems to break styles injected during SSR via useHead 🤔
There was a problem hiding this comment.
Put together an issue; I'm not confident this PR is the root of the issue, but it's my best guess lol
#30188
🔗 Linked issue
📚 Description
this follows up on vuejs/core#6638 to mark layouts as explicit vue components
it should also be slightly more performant 🙏
Summary by CodeRabbit
New Features
Bug Fixes
Documentation