feat(nuxt): add isolate page meta
#29366
Conversation
|
|
| @@ -13,6 +13,7 @@ | |||
| :is="SingleRenderer" | |||
| v-else-if="SingleRenderer" | |||
| /> | |||
| <LazyNuxtPage v-else-if="route.meta?.isolate" /> | |||
There was a problem hiding this comment.
this implementation will result in always bundling <NuxtPage>, even if the vue-router integration isn't enabled.
we should probably protect it so that if no routes have the isolate metadata then there will be no changes to the bundle at all
romhml
force-pushed
the
feat/page-meta-isolate
branch
from
25ac0f1 to
72415c7
Compare
WalkthroughThe changes involve the addition of a new Changes
Assessment against linked issues
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (3)test/bundle.test.ts (3)
The 2kb increase in server bundle size (208k → 210k) is minimal and expected given the new feature. The dependency list remains unchanged, indicating good implementation efficiency.
The 2kb increase in inlined server bundle size (557k → 559k) matches the size increase in the non-inlined version, demonstrating consistent behavior across build configurations.
The 3kb increase in client bundle size (119k → 122k) and addition of Let's verify the contents of the new bundle file: ✅ Verification successfulBundle size increase and new file addition are verified and justified The 3kb increase and addition of
🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Analyze the new isolated-page.js bundle
# Expected: Should contain code related to isolated page functionality
# Look for the isolated page bundle
fd -t f "isolated-page" test/fixtures/minimal/.output/public/_nuxt -x cat {}
Length of output: 208 Script: #!/bin/bash
# Let's first check if the test fixture exists and its structure
fd -t d "minimal" test/fixtures
# Then look for any files related to isolated pages in the test fixtures
fd "isolated" test/fixtures
# Also search for any references to isolated pages in the codebase
rg -g '!{dist,node_modules}' "isolated-page" -A 3
Length of output: 1160 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 (
|
romhml
force-pushed
the
feat/page-meta-isolate
branch
from
72415c7 to
9e0e979
Compare
| export const isolatedPageTemplate: NuxtTemplate = { | ||
| filename: 'isolated-page.mjs', | ||
| getContents (ctx) { | ||
| const hasPages = ctx.nuxt.options.dev || ctx.app.pages?.length |
There was a problem hiding this comment.
I initially planned on using ctx.app.pages?.some((page) => page.meta?.isolate)) but the metas are not completely resolved at this stage. I'm checking if the app has some pages instead which should be fine since this means the vue-router integration is enabled and the NuxtPage component is already bundled.
romhml
force-pushed
the
feat/page-meta-isolate
branch
from
9e0e979 to
d87e72f
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (6)
test/fixtures/basic/pages/not-isolated.vue (1)
1-9: LGTM! Consider adding documentation comments.The component correctly implements a non-isolated page for testing purposes. Consider adding a brief comment explaining its role in testing the isolation feature.
<script setup lang="ts"> +// Test fixture demonstrating a non-isolated page behavior +// Used in conjunction with isolated.vue to verify page isolation functionality definePageMeta({ layout: false, }) </script>test/fixtures/basic/pages/isolated.vue (2)
1-6: LGTM! Consider adding a comment explaining the purpose of this test fixture.The page meta configuration correctly demonstrates the new
isolatefeature with the appropriatelayout: falsesetting.Add a brief comment explaining that this is a test fixture demonstrating isolated page rendering:
<script setup lang="ts"> +// Test fixture demonstrating isolated page rendering without app.vue inheritance definePageMeta({ isolate: true, layout: false, }) </script>
8-10: Consider enhancing the test fixture with more visual indicators.While the current template is functional, adding more visual indicators would make it easier to verify isolation in tests and during development.
Consider this enhancement:
<template> - <div> isolated </div> + <div class="isolated-page"> + <h1>Isolated Page</h1> + <p>This page renders independently without app.vue inheritance</p> + </div> </template> +<style> +/* Styles to verify isolation - should only apply within this component */ +.isolated-page { + padding: 20px; + background: #f0f0f0; + border: 2px dashed #666; +} +</style>docs/3.api/3.utils/define-page-meta.md (1)
92-96: Documentation could be enhanced with more details.While the basic documentation is good, consider enhancing it with:
- Example usage showing how to isolate a page
- Common use cases (e.g., devtools, standalone pages)
- Implications of isolation (what exactly doesn't get inherited)
- Warning about potential side effects
Here's a suggested enhancement:
**`isolate`** - Type: boolean - Set to `true` when you do not want the page to inherit from your `app.vue`. + Set to `true` when you want the page to render independently without inheriting from your `app.vue`. This is useful when: + - Creating standalone pages that need their own root structure + - Implementing tools like devtools that should be isolated from the main app + - Avoiding inheritance of global styles or layouts + + ```vue [pages/standalone.vue] + <script setup lang="ts"> + definePageMeta({ + isolate: true + }) + </script> + ``` + + :::warning + Isolated pages won't inherit any global providers, styles, or layouts from `app.vue`. Ensure your page is self-contained. + :::packages/nuxt/src/core/templates.ts (1)
558-564: LGTM! Clean implementation of isolated page template.The implementation correctly handles both development and production scenarios by checking for the presence of pages. The fallback to
nullwhen no pages exist is a good optimization.The approach of checking for pages instead of
meta.isolateat this stage is correct, as explained in the previous comment, since page metadata is not fully resolved during template generation.test/basic.test.ts (1)
629-634: LGTM! Consider enhancing test coverage.The test case correctly verifies the core functionality of the
meta.isolateproperty by checking both isolated and non-isolated pages. The assertions are clear and focused.Consider adding additional test cases to verify:
- Dynamic route transitions between isolated and non-isolated pages
- Nested layouts behavior with isolated pages
- Edge cases like when
app.vuedoesn't exist
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
docs/3.api/3.utils/define-page-meta.md(2 hunks)packages/nuxt/src/app/components/nuxt-root.vue(2 hunks)packages/nuxt/src/core/templates.ts(2 hunks)packages/nuxt/src/pages/runtime/composables.ts(1 hunks)test/basic.test.ts(1 hunks)test/fixtures/basic/app.vue(1 hunks)test/fixtures/basic/pages/isolated.vue(1 hunks)test/fixtures/basic/pages/not-isolated.vue(1 hunks)
🧰 Additional context used
🪛 GitHub Check: CodeQL
packages/nuxt/src/core/templates.ts
[notice] 13-13: Unused variable, import, function or class
Unused import distDir.
🪛 GitHub Check: code
packages/nuxt/src/core/templates.ts
[failure] 13-13:
'distDir' is defined but never used. Allowed unused vars must match /^_/u
🔇 Additional comments (9)
test/fixtures/basic/app.vue (2)
1-6: LGTM! Clean and minimal test fixture.
The template provides a good base structure for testing the new isolate page meta feature. It demonstrates the default layout inheritance pattern that isolated pages will be able to bypass.
2-5: Verify layout inheritance behavior.
The NuxtLayout wrapper with static content is ideal for testing the isolation feature, as it will make it clear whether a page inherits this layout or not.
Let's verify the related test files:
✅ Verification successful
Let me check the content of these files to verify the layout inheritance behavior.
Based on the test files and test results, I can now verify the layout inheritance behavior:
Layout isolation works correctly.
The test files and results confirm that:
- Pages with
isolate: trueindefinePageMetado not inherit the app layout (<NuxtLayout>wrapper) - Pages without
isolate: trueproperly inherit the app layout
The test results show:
/isolatedrenders only<div>isolated</div>without the<NuxtLayout>wrapper/not-isolatedincludes the<NuxtLayout>wrapper with "Nuxt App" content
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for corresponding test files and isolated page components
# Expected: Find test files verifying isolation behavior and the isolated/non-isolated page components
# Look for related test files
echo "Looking for related test files:"
rg -l "meta.isolate" "test/"
# Look for isolated and non-isolated page components
echo -e "\nLooking for test page components:"
fd -e vue "isolated.vue|not-isolated.vue" "test/"
Length of output: 333
Script:
#!/bin/bash
# Check the content of test files and page components
echo "Content of test/basic.test.ts:"
cat test/basic.test.ts
echo -e "\nContent of test/fixtures/basic/pages/isolated.vue:"
cat test/fixtures/basic/pages/isolated.vue
echo -e "\nContent of test/fixtures/basic/pages/not-isolated.vue:"
cat test/fixtures/basic/pages/not-isolated.vue
Length of output: 119858
test/fixtures/basic/pages/not-isolated.vue (1)
2-4: Clarify the purpose of layout: false.
The layout: false setting might be confused with the new isolation feature. Consider documenting why this setting is needed in this test case, or verify if it's necessary at all.
✅ Verification successful
Let me gather more context about these test files to understand the purpose of layout: false.
Let me check if there's any documentation or test setup that explains the purpose of these test files.
Let me check the page-extend module to understand the context better.
Based on the gathered context, I can now provide a final response:
layout: false is correctly used in this test fixture
The layout: false setting in these test files serves a specific testing purpose:
- In
internal-layout.vue, it disables the default layout to implement a custom layout logic using<NuxtLayout>component - In
isolated.vueandnot-isolated.vue, it's used to test pages without any layout interference, allowing clean comparison of the isolation feature - This pattern is consistently used across test fixtures, including programmatically added pages in the page-extend module
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other test fixtures using layout: false to understand the testing pattern
rg -g '*.vue' -B 2 -A 2 'layout:\s*false'
Length of output: 1248
Script:
#!/bin/bash
# Check the content of these test files to understand their testing purpose
echo "=== isolated.vue ==="
cat test/fixtures/basic/pages/isolated.vue
echo -e "\n=== not-isolated.vue ==="
cat test/fixtures/basic/pages/not-isolated.vue
echo -e "\n=== internal-layout.vue ==="
cat test/fixtures/basic/pages/internal-layout.vue
Length of output: 836
Script:
#!/bin/bash
# Check for test setup or documentation
echo "=== Test Directory README ==="
cat test/fixtures/basic/README.md 2>/dev/null || echo "No README found"
echo -e "\n=== Test Setup ==="
cat test/fixtures/basic/test.spec.ts 2>/dev/null || echo "No test.spec.ts found"
echo -e "\n=== Searching for layout-related tests ==="
rg -g '*.{ts,js}' 'layout.*false' test/
Length of output: 581
Script:
#!/bin/bash
# Check the page-extend module implementation
echo "=== Page Extend Module ==="
cat test/fixtures/basic/modules/page-extend/index.ts
echo -e "\n=== Looking for layout-related tests ==="
find test/fixtures/basic -type f -name "*.test.ts" -o -name "*.spec.ts" | while read -r file; do
echo "=== $file ==="
cat "$file"
done
Length of output: 1113
test/fixtures/basic/pages/isolated.vue (1)
1-10: Verify test coverage for isolated page rendering.
Let's ensure there are corresponding test cases that verify the isolation behavior.
✅ Verification successful
Based on the search results, I can see there is a test case specifically for isolated page rendering in test/basic.test.ts. Let me verify its implementation:
Test coverage for isolated page rendering is properly implemented
The test case in test/basic.test.ts comprehensively verifies the isolation behavior by:
- Checking that non-isolated pages contain the app wrapper ("Nuxt App")
- Verifying that isolated pages do not contain the app wrapper
- Confirming the isolated page content is correctly rendered
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for test cases related to isolated page rendering
# Look for test cases specifically testing the isolated page
echo "Searching for isolated page test cases..."
rg -l "isolated" test/basic.test.ts
# Look for assertions related to isolation testing
echo "Searching for isolation-related assertions..."
rg "test|describe|it.*isola" test/
Length of output: 43750
Script:
#!/bin/bash
# Check the specific test case for isolated page rendering
rg -A 5 "should render pages with meta.isolate independently" test/basic.test.ts
Length of output: 350
packages/nuxt/src/app/components/nuxt-root.vue (2)
50-52: LGTM! Clean route handling implementation.
The explicit route definition and provision to PageRouteSymbol is a good practice that improves code clarity and maintainability.
16-19: Verify complete isolation from app.vue.
While the implementation looks correct, we should verify that isolated pages are truly independent and don't inherit any context or styles from app.vue.
Let's check for any potential leaks:
Also applies to: 50-54
✅ Verification successful
Let me try searching with the correct paths.
Let me check one more aspect to verify the isolation mechanism.
Based on the gathered information, I can now provide the final response:
Isolation from app.vue is properly implemented
The implementation ensures complete isolation through several mechanisms:
- Isolated pages are loaded via a separate build artifact (
isolated-page.mjs) - The only shared context is the route symbol (
PageRouteSymbol), which is necessary for navigation - No global styles, provide/inject patterns, or other potential leaks from app.vue are present
- The build system handles isolated pages independently from the main app component
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential inheritance or context leaks from app.vue to isolated pages
# Test 1: Look for provide/inject usage in app.vue that might affect isolated pages
echo "Checking for provide/inject in app.vue..."
rg -A 5 "provide\(" "app.vue"
# Test 2: Check for global styles or assets that might leak
echo "Checking for style imports..."
rg -A 5 "style|css" "app.vue"
# Test 3: Look for any composables used in both app.vue and isolated pages
echo "Checking for shared composables..."
ast-grep --pattern "use[A-Z][a-zA-Z]*\(" "app.vue"
Length of output: 502
Script:
#!/bin/bash
# Description: Check for potential inheritance or context leaks from app.vue to isolated pages
# Test 1: Look for provide/inject usage in app.vue that might affect isolated pages
echo "Checking for provide/inject in app.vue..."
fd -e vue app.vue --exec rg -A 5 "provide\(" {}
# Test 2: Check for global styles or assets that might leak
echo "Checking for style imports..."
fd -e vue app.vue --exec rg -A 5 "style|css" {}
# Test 3: Look for any composables used in both app.vue and isolated pages
echo "Checking for shared composables..."
fd -e vue app.vue --exec ast-grep --pattern "use[A-Z][a-zA-Z]*\(" {}
# Test 4: Check nuxt-root.vue for any potential leaks
echo "Checking nuxt-root.vue for potential leaks..."
fd -e vue nuxt-root.vue --exec rg -A 5 "(provide|inject|style|css)" {}
# Test 5: Check if there's any direct app.vue inheritance
echo "Checking for app.vue inheritance..."
fd -e vue nuxt-root.vue --exec rg -A 5 "app\.vue" {}
Length of output: 1502
Script:
#!/bin/bash
# Check the isolated page implementation and its relationship with app.vue
echo "Checking isolated page implementation..."
rg -A 10 "isolate" --type vue
# Check how the isolated page is loaded
echo "Checking isolated page loading mechanism..."
rg -A 10 "isolated-page" --type ts --type vue
# Check for any potential app.vue references in the build system
echo "Checking build system for app.vue handling..."
rg -A 5 "app-component|isolated-page" --type ts
Length of output: 1376
packages/nuxt/src/pages/runtime/composables.ts (2)
47-49: LGTM! Consider adding runtime validation.
The new isolate property is well-documented and properly typed. The implementation aligns with the PR objectives to allow pages to be rendered independently of app.vue.
Let's verify if there are any existing usages of this property that might need validation:
#!/bin/bash
# Search for any existing usage of the isolate property in page meta definitions
rg -l "definePageMeta.*isolate.*true" || echo "No existing usage found"Line range hint 52-55: LGTM! Verify route meta compatibility.
The RouteMeta interface extension is correctly implemented, making the new isolate property available in vue-router's type system.
Let's verify if there are any existing custom route meta implementations that might be affected:
✅ Verification successful
The previous ast-grep pattern had a syntax error. Let's try with ripgrep to:
- Find files containing RouteMeta interface declarations
- Look for actual route meta usage patterns in the codebase to understand existing implementations
Let's try with simpler regex patterns to:
- Confirm there are no other RouteMeta interface declarations
- Find actual route meta usage in the codebase
- Look for page meta definitions that might be affected
Based on the search results, I can now generate the final response:
The RouteMeta interface extension is safe to implement
The search results confirm that:
- The
RouteMetainterface is only declared once in the codebase (in composables.ts) - All route meta properties are defined through
definePageMeta()which is properly typed withPageMeta - The test files show that the type system correctly validates meta properties
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for custom RouteMeta interface extensions or usages
ast-grep --pattern 'interface RouteMeta {
$$$
}'
Length of output: 146
Script:
#!/bin/bash
# Search for RouteMeta interface extensions and usages with ripgrep
rg -l "interface\s+RouteMeta" --type ts
# Search for route meta usage patterns
rg "meta:\s*{" --type ts -A 2
Length of output: 202
Script:
#!/bin/bash
# Search for RouteMeta interface extensions
rg -l "interface RouteMeta" --type ts
# Search for route meta usage with fixed regex
rg "meta: {" --type ts -A 2
# Also search for definePageMeta usage which might define meta properties
rg "definePageMeta" --type ts -A 2
Length of output: 11402
docs/3.api/3.utils/define-page-meta.md (1)
41-41: LGTM! Interface addition is well-typed.
The isolate property is correctly added as an optional boolean to the PageMeta interface.
| <component | ||
| :is="IsolatedPage" | ||
| v-else-if="IsolatedPage && route?.meta?.isolate" | ||
| /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider optimizing bundle size for isolated page feature.
While the conditional rendering logic is correct, importing IsolatedPage unconditionally might increase the base bundle size even when the feature isn't used. Consider implementing dynamic imports based on route metadata to avoid unnecessary code in the bundle.
Here's a suggested approach:
-const IsolatedPage = defineAsyncComponent(() => import('#build/isolated-page.mjs'))
+const IsolatedPage = computed(() => {
+ if (route?.meta?.isolate) {
+ return defineAsyncComponent(() => import('#build/isolated-page.mjs'))
+ }
+ return null
+})Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Not sure if it's the best solution in term of performance here
There was a problem hiding this comment.
This can probably just be
const IsolatedPage = route?.meta?.isolate ? defineAsyncComponent(() => import('#build/isolated-page.mjs')) : null
There was a problem hiding this comment.
@huang-julien, thank you for your suggestion. Your approach simplifies the conditional loading of IsolatedPage and helps to optimize performance by importing the component only when route.meta.isolate is true. This should effectively reduce the bundle size when the isolated page feature isn't used.
There was a problem hiding this comment.
@huang-julien, thank you for the clarification. Your suggested approach is indeed more efficient. Using the inline conditional import avoids unnecessary code in the bundle without the overhead of a computed property. I'll keep this in mind for future reviews.
✏️ Learnings added
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.745Z
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.
> Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure [review instructions](https://docs.coderabbit.ai/guides/review-instructions). You can manage existing learnings in the [app](https://app.coderabbit.ai/learnings).
</details>
<!-- This is an auto-generated reply by CodeRabbit -->
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/nuxt/src/core/templates.ts (1)
557-563: Consider improving path resolution and condition check.The implementation looks good but could be enhanced in two areas:
- The path resolution using
../pages/runtime/pagecould be made more robust by using a dedicated alias or constant.- The condition
ctx.app.pages?.lengthmight be too broad as it doesn't specifically check for isolated pages.Consider these improvements:
export const isolatedPageTemplate: NuxtTemplate = { filename: 'isolated-page.mjs', getContents (ctx) { - const hasPages = ctx.nuxt.options.dev || ctx.app.pages?.length - return hasPages ? genExport(resolve(ctx.nuxt.options.appDir, '../pages/runtime/page'), ['default']) : 'export default null' + const hasIsolatedPages = ctx.nuxt.options.dev || ctx.app.pages?.some(page => page.meta?.isolate) + const pagePath = '#pages/runtime/page' // Using an alias would be more maintainable + return hasIsolatedPages ? genExport(pagePath, ['default']) : 'export default null' }, }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/nuxt/src/core/templates.ts(1 hunks)
🔇 Additional comments (1)
packages/nuxt/src/core/templates.ts (1)
560-560: Acknowledging the implementation choice.
I see from the past review comments that checking for page.meta?.isolate directly isn't feasible as metas aren't completely resolved at this stage. The current implementation using ctx.app.pages?.length is a reasonable approach given this constraint.
|
I'm thinking one way to do this would be
Would this be acceptable? It would basically hook into existing functionality (but make it more secure by requiring a defined list of components that could be rendered this way). It would also mean you would access this component at |
| @@ -13,6 +13,10 @@ | |||
| :is="SingleRenderer" | |||
| v-else-if="SingleRenderer" | |||
| /> | |||
| <component | |||
| :is="IsolatedPage" | |||
| v-else-if="IsolatedPage && route?.meta?.isolate" | |||
There was a problem hiding this comment.
Isolated page will always be truthy because it is an async loader
|
Thanks for the suggestion @danielroe! This should work perfectly for my use-case, but I'm thinking it might be a bit specific. The current implementation is a bit more flexible and could be reused by other module authors to inject pages (for devtools, auth, etc.). What's bothering you with the current solution? I'll look into implementing your suggestion so we can compare. |
|
Although it might seem less flexible to implement |
|
Thanks, I understand. We can close this PR then, I'll work on implementing it using |
🔗 Linked issue
resolves #29365
📚 Description
This PR introduces a new page meta
isolateto render pages without inheriting from theapp.vuecomponent.Summary by CodeRabbit
Release Notes
New Features
isolateproperty in thePageMetainterface, allowing pages to render independently from the default layout.isolated.vueandnot-isolated.vue, demonstrating the use of theisolateproperty.Bug Fixes
Tests
Documentation
definePageMetafunction to include the newisolateproperty.