feat(nuxt): support local functions in definePageMeta #30241
Conversation
|
|
WalkthroughThe pull request introduces significant improvements to Nuxt's code transformation and scope tracking mechanisms across multiple files. The changes primarily focus on enhancing the handling of variable scopes, identifiers, and metadata processing. In The In Assessment against linked issues
Sequence DiagramsequenceDiagram
participant Parser as Code Parser
participant ScopeTracker as Scope Tracker
participant Transformer as Code Transformer
Parser->>ScopeTracker: Create Scope
Parser->>ScopeTracker: Track Identifiers
ScopeTracker-->>Transformer: Provide Scope Information
Transformer->>Transformer: Process Metadata
Transformer->>ScopeTracker: Verify Identifiers
The sequence diagram illustrates the new flow of scope tracking and code transformation, highlighting the improved interaction between the parser, scope tracker, and transformer. 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 (
|
…page-meta' into feat-support-local-functions-in-page-meta
|
Initially, I wanted to extract only the functions that don’t reference anything outside their block scope and throw an error otherwise. However, I quickly noticed this approach wouldn’t work, as it treated JavaScript globals like Another feature I wanted to add support for was the use of imported functions. I found out that this is already supported, but we’re not currently checking for any side effects in this situation either. Do you think it’s worth exploring further, or should we leave it as is, like the previous point? |
cernymatej
force-pushed
the
feat-support-local-functions-in-page-meta
branch
from
e55682b to
1863f8e
Compare
|
The new scope tracker should also address some issues with key injection. Consider the following example: function foo(useAsyncData: (key?: any) => void) {
useAsyncData()
}
foo((key) => {
console.log(key)
})In the current version of Nuxt, the key would be injected here because the original tracker didn’t account for function parameters, among other things. That said, I might have missed something too, so I’d be very grateful for a review. 🙏 |
![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/core/plugins/composable-keys.ts (1)
Line range hint
55-77: Consider refactoring shared scope tracking logicThe scope tracking implementation mirrors functionality in
packages/nuxt/src/pages/plugins/page-meta.ts. Refactoring common logic into a shared utility could improve maintainability and reduce code duplication.packages/nuxt/src/pages/plugins/page-meta.ts (1)
161-216: Consider refactoring shared logic for scope trackingSimilar scope tracking and declaration processing exist in
packages/nuxt/src/core/plugins/composable-keys.ts. Extracting shared functionality into a common utility could enhance code reuse and simplify maintenance.packages/nuxt/test/page-metadata.test.ts (1)
Line range hint
77-94: Complete the test case for experimental decoratorsThe test for handling experimental decorators is marked as TODO. Implementing this test will help ensure robust support for decorators in
definePageMeta.Do you need assistance in writing this test case or addressing any challenges related to experimental decorators?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/nuxt/src/core/plugins/composable-keys.ts(3 hunks)packages/nuxt/src/core/utils/parse.ts(3 hunks)packages/nuxt/src/pages/plugins/page-meta.ts(2 hunks)packages/nuxt/test/page-metadata.test.ts(1 hunks)
🔇 Additional comments (24)
packages/nuxt/src/core/plugins/composable-keys.ts (6)
9-9: Import necessary utilities for scope tracking
The addition of ScopeTracker, parseAndWalk, and walk from '../utils/parse' is appropriate for enhanced scope management and parsing functionalities.
55-57: Instantiate ScopeTracker with keepExitedScopes option
Creating a new ScopeTracker with { keepExitedScopes: true } ensures that scopes are preserved after exiting, which is crucial for variable declaration retrieval in subsequent traversals.
59-59: Pass scopeTracker to parseAndWalk
Providing the scopeTracker to the parseAndWalk function allows for accurate scope tracking during the initial AST traversal.
62-63: Freeze scopeTracker after initial traversal
Calling scopeTracker.freeze() prevents further modifications to the tracked scopes, ensuring that scope information remains consistent for later use.
65-65: Include scopeTracker in the second AST traversal
Passing scopeTracker to the walk function maintains scope information during the transformation phase.
76-77: Retrieve declarations using scopeTracker.getDeclaration(name)
Using scopeTracker.getDeclaration(name) efficiently retrieves variable declarations within the tracked scopes, enhancing the accuracy of identifier resolution.
packages/nuxt/src/pages/plugins/page-meta.ts (6)
11-17: Import utilities for advanced scope management
Adding imports for ScopeTracker and related types from '../../core/utils/parse' is essential for the updated scope tracking and undeclared identifier handling.
128-131: Implement isStaticIdentifier to check import map
The isStaticIdentifier function correctly determines if a name exists in the importMap, ensuring accurate identification of static identifiers.
132-139: Optimize import additions with addImport function
The addImport function effectively adds imports without duplication by checking the addedImports set.
140-149: Manage declarations using addDeclaration
The addDeclaration function ensures that variable declarations are tracked and avoids duplicates by utilizing the addedDeclarations set.
150-159: Handle identifiers with addImportOrDeclaration
The addImportOrDeclaration function appropriately handles identifiers by adding them as imports or declarations based on their presence in the importMap.
163-193: Process declarations in processDeclaration function
The processDeclaration function effectively processes different node types, recursively handling identifiers and their dependencies.
packages/nuxt/src/core/utils/parse.ts (11)
3-14: Import required ESTree types for comprehensive AST manipulation
Including additional ESTree types supports the extended functionality of node handling and scope tracking in the parser utilities.
23-28: Define WalkOptions for flexible AST traversal
Introducing the WalkOptions interface allows for optional callbacks and integrates scopeTracker, enhancing the flexibility of the walk function.
29-41: Update walk function to use WalkOptions
Modifying the walk function to accept Partial<WalkOptions> improves its configurability and supports the extended scope tracking features.
45-46: Align parseAndWalk with updated walk function
Adjusting parseAndWalk to accept Partial<WalkOptions> maintains consistency and compatibility with the updated walk function.
56-88: Introduce base classes for structured node handling
Defining BaseNode and subclasses like IdentifierNode and FunctionParamNode provides a solid foundation for representing various AST node types in scope tracking.
89-172: Add specialized node classes for scope tracking
Implementing classes like FunctionNode, VariableNode, ImportNode, and CatchParamNode enhances the ability to track different declaration types accurately.
173-434: Enhance ScopeTracker for comprehensive scope management
The expanded ScopeTracker class efficiently tracks variable declarations and scopes, improving the accuracy of undeclared identifier detection.
436-468: Implement getPatternIdentifiers utility function
The getPatternIdentifiers function correctly extracts identifiers from various pattern nodes, aiding in scope declaration.
470-511: Define isNotReferencePosition to filter identifier usage
This function accurately identifies positions where an identifier is not a reference, which is vital for correct scope analysis.
513-531: Provide getUndeclaredIdentifiersInFunction utility
This utility effectively identifies undeclared identifiers within a function, enhancing error detection and scope resolution.
173-434: Consider consolidating scope tracking logic
The scope tracking mechanisms implemented here resemble those in other parts of the codebase. Refactoring common logic into a shared module could improve maintainability.
packages/nuxt/test/page-metadata.test.ts (1)
306-308: Validate code transformation in metadata extraction tests
The test correctly verifies the transformed code for metadata extraction, ensuring that the definePageMeta macro functions as expected within Vue components.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/nuxt/src/pages/plugins/page-meta.ts (3)
128-130: Consider strengthening type safetyThe type predicate function could be more explicit about the string type.
-function isStaticIdentifier (name: string | false): name is string { +function isStaticIdentifier (name: string | false): name is NonNullable<string> {
171-174: Enhance error message for await expressionsThe error message could be more specific about why await expressions aren't supported.
- logger.error(`[nuxt] Await expressions are not supported in definePageMeta. File: '${id}'`) + logger.error(`[nuxt] Await expressions are not supported in definePageMeta as they may lead to runtime errors. File: '${id}'`)
163-193: Consider memory optimisation for large codebasesThe declaration processing logic looks solid, but for large codebases, consider implementing a cleanup mechanism for the
addedDeclarationsSet.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/nuxt/src/pages/plugins/page-meta.ts(2 hunks)
🔇 Additional comments (4)
packages/nuxt/src/pages/plugins/page-meta.ts (4)
11-17: LGTM: Well-structured imports for scope tracking functionality
The new imports are properly organised and specifically scoped to the required functionality.
161-161: LGTM: Appropriate scope tracker initialization
The ScopeTracker instance is properly initialised at module scope, making it available to all relevant functions.
217-223: LGTM: Well-structured code generation
The code generation logic properly handles both imports and declarations, maintaining their original order.
195-229: Verify handling of template literals in metadata
The transformation looks robust, but we should verify the handling of empty template literals in metadata.
✅ Verification successful
Based on the extensive search results from the previous commands, I can now generate the final response:
Template literals in metadata are handled correctly
The code transformation in page-meta.ts correctly handles all types of string values in definePageMeta. From examining the test files and actual usage across the codebase:
- No instances of template literals in
definePageMetawere found in real usage - The transformation logic uses
code.slice(meta.start, meta.end)which preserves any string format - Test cases cover various string formats including quoted strings and object properties
- The compiler macro properly serialises the metadata during build time
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of template literals in definePageMeta calls
ast-grep --pattern 'definePageMeta({
$$$
$_: `$_`
$$$
})'
Length of output: 65
Script:
#!/bin/bash
# Let's try a broader search for definePageMeta usage patterns
rg "definePageMeta" -A 5 -B 5
# Also search for test files that might contain template literal examples
fd -e test.ts -e spec.ts -X rg "definePageMeta"
# And check the documentation for any examples
fd -e md -X rg "definePageMeta"
Length of output: 67191
Script:
#!/bin/bash
# Let's search for template literals specifically in definePageMeta calls
rg -U "definePageMeta\(\{[^}]*\`[^`]*\`[^}]*\}\)"
# Also search for any string interpolation in definePageMeta
rg -U "definePageMeta\(\{[^}]*\$\{[^}]*\}[^}]*\}\)"
Length of output: 116
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/nuxt/src/core/utils/parse.ts (1)
32-33: Consider removing @ts-expect-errorInstead of suppressing the TypeScript error, consider extending the type definition to properly type the
scopeTrackerproperty access.Also applies to: 37-38
packages/nuxt/test/fixture/scope-tracker.ts (1)
3-15: Add JSDoc documentation for the test utility classWhilst the implementation is correct, adding JSDoc documentation would improve maintainability by clarifying the testing purpose of these methods and their expected behaviour.
Add documentation like this:
+/** + * Test utility class that provides access to internal ScopeTracker properties. + * This class should only be used in tests. + */ export class TestScopeTracker extends ScopeTracker { + /** + * Returns the internal scopes Map for testing purposes. + */ getScopes () { return this.scopes } + /** + * Returns the current scope index key for testing purposes. + */ getScopeIndexKey () { return this.scopeIndexKey } + /** + * Returns the scope index stack for testing purposes. + */ getScopeIndexStack () { return this.scopeIndexStack } }packages/nuxt/test/parse.test.ts (2)
42-216: Consider breaking down the large scope key testWhilst the test is thorough, its size makes it challenging to maintain. Consider splitting it into smaller, focused test cases for each scope scenario (functions, loops, classes, etc.).
Example structure:
describe('scope key generation', () => { it('should handle function scopes correctly', () => { // Test function-related scopes }) it('should handle loop scopes correctly', () => { // Test loop-related scopes }) it('should handle class scopes correctly', () => { // Test class-related scopes }) })
278-321: Consider adding edge cases to pattern handling testsWhilst the current test covers basic patterns well, consider adding edge cases such as:
- Nested destructuring with default values
- Rest parameters in patterns
- Complex array destructuring patterns
Example additions:
const code = ` const { a = 1, b: { c = 2 } = {} } = {} const [d, ...rest] = [1, 2, 3] const [e, , f] = [1, 2, 3] `
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/nuxt/src/core/utils/parse.ts(3 hunks)packages/nuxt/test/fixture/scope-tracker.ts(1 hunks)packages/nuxt/test/parse.test.ts(1 hunks)
🔇 Additional comments (8)
packages/nuxt/src/core/utils/parse.ts (4)
23-27: Interface design looks good
The WalkOptions interface is well-structured and includes all necessary components for scope tracking.
437-469: Pattern matching implementation is thorough
The getPatternIdentifiers function handles all relevant JavaScript pattern matching cases comprehensively.
471-512: Comprehensive reference position checking
The isNotReferencePosition function thoroughly handles all edge cases for identifying non-reference positions in the AST.
217-435: Well-implemented scope tracking with comprehensive documentation
The ScopeTracker class is well-documented and implements a robust scope tracking system. The implementation correctly handles various JavaScript scoping rules and maintains proper scope hierarchies.
Let's verify the scope tracking implementation:
✅ Verification successful
Scope tracking implementation is robust and properly implemented
The verification confirms that the scope tracking implementation is sound with:
- Proper scope management through balanced
pushScope/popScopecalls - Correct cleanup of exited scopes via
scopes.delete - Well-implemented scope chain traversal for identifier lookups
- Appropriate scope freezing mechanism for preventing further declarations
- Proper handling of scope indices and key management
The implementation correctly maintains lexical scoping rules for:
- Function declarations and expressions
- Block scopes
- Class declarations
- Import declarations
- Loop constructs
- Catch clauses
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify scope tracking implementation
# Test: Check for any potential infinite loops in scope tracking
# Look for recursive calls without proper exit conditions
ast-grep --pattern 'class ScopeTracker {
$$$
$_($$_) {
$$$
this.$_($$$)
$$$
}
$$$
}'
Length of output: 106
Script:
#!/bin/bash
# Let's verify the scope tracking implementation by checking:
# 1. Method calls between scope-related methods
# 2. Proper scope cleanup
# Check for scope-related method calls
rg "this\.(push|pop)Scope|this\.updateScopeIndexKey|this\.declare" packages/nuxt/src/core/utils/parse.ts -A 2 -B 2
# Check for potential memory leaks in scope management
rg "scopes\.(delete|set|get)" packages/nuxt/src/core/utils/parse.ts -A 2 -B 2
# Verify scope index management
rg "scopeIndexStack\." packages/nuxt/src/core/utils/parse.ts -A 2 -B 2
Length of output: 5324
packages/nuxt/test/parse.test.ts (4)
7-24: LGTM! Well-structured test for basic scope behaviour
The test effectively validates the core functionality of scope tracking, demonstrating that scopes are properly discarded when keepExitedScopes is false.
25-41: LGTM! Comprehensive test for scope retention
The test properly validates the scope retention behaviour when keepExitedScopes is true.
217-244: LGTM! Clear validation of variable declaration tracking
The test effectively demonstrates the isolation of variables in different scopes.
245-277: LGTM! Thorough validation of variable separation
The test properly verifies that variables with the same name remain distinct across different scopes.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
packages/nuxt/test/parse.test.ts (3)
8-23: Consider adding edge cases to scope exit test.While the basic test is good, consider adding edge cases such as nested blocks, early returns, and try-catch-finally blocks to ensure robust scope handling.
42-215: Consider splitting the large scope key test.This test case is quite large and tests multiple aspects of scope key generation. Consider breaking it down into smaller, focused test cases for better maintainability and easier debugging.
Example structure:
describe('scope key generation', () => { it('should handle function scopes correctly', () => { // Test function-related scope keys }) it('should handle loop scopes correctly', () => { // Test loop-related scope keys }) it('should handle class scopes correctly', () => { // Test class-related scope keys }) })
296-352: Add tests for complex destructuring patterns.The pattern handling tests could be expanded to cover more complex cases such as nested destructuring and default values.
Example additions:
it('should handle complex destructuring patterns', () => { const code = ` const { a: { b: { c = 1 } = {} } = {} } = obj function foo({ a: [b, { c: d = 2 } = {}] = [] } = {}) { } ` // Test assertions })packages/nuxt/src/core/utils/parse.ts (2)
251-260: Consider adding error handling for frozen state.The
declareIdentifiermethod silently returns when the tracker is frozen. Consider throwing an error or logging a warning to help with debugging.protected declareIdentifier (name: string, data: ScopeTrackerNode) { - if (this.isFrozen) { return } + if (this.isFrozen) { + console.warn(`Attempted to declare identifier "${name}" in frozen scope tracker`) + return + }
522-540: Consider caching results of getUndeclaredIdentifiersInFunction.This function could be expensive for large functions. Consider caching the results using a WeakMap.
const undeclaredCache = new WeakMap<Node, string[]>() export function getUndeclaredIdentifiersInFunction(node: FunctionDeclaration | FunctionExpression | ArrowFunctionExpression) { const cached = undeclaredCache.get(node) if (cached) return cached // ... existing implementation ... undeclaredCache.set(node, Array.from(undeclaredIdentifiers)) return undeclaredCache.get(node)! }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/nuxt/src/core/utils/parse.ts(3 hunks)packages/nuxt/test/fixture/scope-tracker.ts(1 hunks)packages/nuxt/test/parse.test.ts(1 hunks)
🔇 Additional comments (1)
packages/nuxt/test/fixture/scope-tracker.ts (1)
1-31: LGTM! Well-structured test fixture class.
The TestScopeTracker class is well-designed for testing purposes, providing controlled access to internal state through focused methods. The implementation follows testing best practices by exposing only what's necessary for verification.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/nuxt/src/core/utils/parse.ts (2)
445-477: Consider adding memoization for pattern identifier collection.For complex patterns that might be processed multiple times, consider caching the results using a WeakMap to improve performance.
+const patternCache = new WeakMap<Node, WithLocations<Identifier>[]>() function getPatternIdentifiers (pattern: WithLocations<Node>) { + const cached = patternCache.get(pattern) + if (cached) return cached + const identifiers: WithLocations<Identifier>[] = [] // ... existing implementation ... collectIdentifiers(pattern) + patternCache.set(pattern, identifiers) return identifiers }
479-528: Consider adding tests for edge cases in reference position detection.The function handles various cases comprehensively, but some edge cases might need explicit test coverage:
- Computed property names
- Object shorthand properties
- Method shorthand
- Nested patterns
Would you like me to help generate additional test cases for these scenarios?
packages/nuxt/test/parse.test.ts (1)
1-512: Excellent test coverage with room for enhancement.The test suite is comprehensive and well-structured. Consider adding these additional test cases:
- Dynamic imports
- Computed class property names
- Generator and async functions
- Default parameters with complex patterns
Would you like me to help generate these additional test cases?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/nuxt/src/core/utils/parse.ts(3 hunks)packages/nuxt/test/parse.test.ts(1 hunks)
🔇 Additional comments (5)
packages/nuxt/src/core/utils/parse.ts (5)
Line range hint 3-27: LGTM! Well-structured imports and interface definition.
The imports are specific and well-organised, and the WalkOptions interface clearly defines the structure for walk callbacks with scope tracking support.
32-38: Replace @ts-expect-error comments with proper type guards.
Instead of suppressing TypeScript errors, consider properly typing the scopeTracker property access or creating a type guard.
56-171: Excellent class hierarchy design!
The class structure is well-organised with:
- Clear abstract base class with documented abstract methods
- Consistent implementation pattern across derived classes
- Strong typing and proper encapsulation
403-415: Optimise scope lookup in isDeclared method.
The current implementation splits and joins strings repeatedly in the loop. Consider caching the scope keys.
530-557: Excellent implementation of undeclared identifier detection!
The two-pass approach with scope freezing is a clever solution that correctly handles:
- Variable hoisting
- Function declarations
- Scope boundaries
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/nuxt/src/pages/plugins/page-meta.ts(2 hunks)packages/nuxt/test/page-metadata.test.ts(2 hunks)
🔇 Additional comments (11)
packages/nuxt/test/page-metadata.test.ts (4)
4-4: Importing 'esbuildTransform' correctly
The import statement for esbuildTransform from esbuild is appropriate and necessary for the transformations in the test cases.
314-352: Test for local function extraction is correctly implemented
The test case effectively verifies that local functions can be used within definePageMeta. It correctly simulates the scenario and checks the expected output, ensuring that local functions are handled properly during the code transformation process.
384-433: Test for 'esbuild.keepNames = true' is correctly implemented
The test accurately verifies that function names are preserved when keepNames is set to true in esbuild. This ensures that the code transformation maintains the correct naming, which is crucial for runtime behaviour, especially when function names are used in metadata.
435-469: Test for error handling with 'await' expressions is correctly implemented
The test appropriately checks that an error is thrown when await expressions are used within definePageMeta, which is unsupported. It ensures that the error handling mechanism is functioning as expected.
packages/nuxt/src/pages/plugins/page-meta.ts (7)
11-17: Import statements are correctly updated
The necessary imports for scope tracking and parsing utilities have been added from '../../core/utils/parse'. This sets up the required functionality for improved scope management in the plugin.
128-130: Utility function 'isStaticIdentifier' is correctly implemented
The function isStaticIdentifier accurately determines whether a given name corresponds to a static import, which is essential for correctly handling imported identifiers during code transformation.
132-138: 'addImport' function correctly adds static imports
The addImport function effectively adds the import statement to addedImports if it hasn't been added already, preventing duplicate imports and ensuring all necessary imports are included.
140-148: 'addDeclaration' function correctly tracks declarations
The addDeclaration function ensures that variable and function declarations are added only once to declarationNodes, avoiding duplication and maintaining an accurate list of declarations needed for the transformation.
150-159: 'addImportOrDeclaration' function correctly handles imports and declarations
The function appropriately adds static imports or processes declarations based on whether the identifier is a static import or a local declaration, ensuring all dependencies are correctly managed.
163-193: 'processDeclaration' function effectively handles variable and function declarations
The function processDeclaration correctly processes variable and function declarations, recursively handling dependencies and appropriately logging errors for unsupported constructs like AwaitExpression.
195-231: Parsing and code transformation logic is properly implemented
The code transformation logic effectively traverses the AST, collects necessary imports and declarations, and reconstructs the code to include the extracted metadata. This ensures that the definePageMeta function works correctly with local functions and imports.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
docs/2.guide/2.directory-structure/1.pages.md (2)
269-271: Hyphenate "meta-object" for consistencyThe technical explanation is accurate and helpful. Consider hyphenating "meta-object" for consistency with technical documentation standards.
-Therefore, the page meta object cannot reference the component. +Therefore, the page meta-object cannot reference the component.🧰 Tools
🪛 LanguageTool
[misspelling] ~270-~270: This word is normally spelled as one.
Context: ...t of the component. Therefore, the page meta object cannot reference the component. However...(EN_COMPOUNDS_META_OBJECT)
[uncategorized] ~271-~271: Loose punctuation mark.
Context: ...as locally defined pure functions. ::warning Make sure not to reference any ...(UNLIKELY_OPENING_PUNCTUATION)
280-282: Consider adding TypeScript types for better type safetyThe validation function example is clear, but could benefit from TypeScript types for better maintainability and IDE support.
-function validateIdParam(route) { +function validateIdParam(route: RouteLocationNormalized) { return route.params.id && !isNaN(Number(route.params.id)) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/2.guide/2.directory-structure/1.pages.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/2.guide/2.directory-structure/1.pages.md
[misspelling] ~270-~270: This word is normally spelled as one.
Context: ...t of the component. Therefore, the page meta object cannot reference the component. However...
(EN_COMPOUNDS_META_OBJECT)
[uncategorized] ~271-~271: Loose punctuation mark.
Context: ...as locally defined pure functions. ::warning Make sure not to reference any ...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~273-~273: Loose punctuation mark.
Context: .... This can lead to unexpected behavior. :: ```vue <script setup lang="ts"> impor...
(UNLIKELY_OPENING_PUNCTUATION)
🔇 Additional comments (2)
docs/2.guide/2.directory-structure/1.pages.md (2)
272-274: LGTM! Clear and important warning
The warning effectively communicates the limitations regarding reactive data and side effects.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~273-~273: Loose punctuation mark.
Context: .... This can lead to unexpected behavior. :: ```vue <script setup lang="ts"> impor...
(UNLIKELY_OPENING_PUNCTUATION)
285-286: LGTM! Clear example of local function usage
The example effectively demonstrates how to use locally defined functions and imported data with definePageMeta.
|
|
||
| definePageMeta({ | ||
| title, // This will create an error |
There was a problem hiding this comment.
I think it's worth preserving, as this example is a warning example...
There was a problem hiding this comment.
It won't create an error now, though. The ref will be extracted too
const title = ref("");
const __nuxt_page_meta = {
title
}
export default __nuxt_page_metaIt doesn't really make sense to use a ref in this way, since it will be a different ref than the one in the component - hence the warning "Make sure not to reference any reactive data"
I've added it back with a different comment
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
docs/2.guide/2.directory-structure/1.pages.md (2)
269-271: Minor text improvements neededConsider these refinements to improve clarity and consistency:
- Hyphenate "meta object" to "meta-object" for consistency with technical terminology
- Consider adding a brief example of an imported binding for better clarity
-Therefore, the page meta object cannot reference the component. +Therefore, the page meta-object cannot reference the component.🧰 Tools
🪛 LanguageTool
[misspelling] ~270-~270: This word is normally spelled as one.
Context: ...t of the component. Therefore, the page meta object cannot reference the component. However...(EN_COMPOUNDS_META_OBJECT)
[uncategorized] ~271-~271: Loose punctuation mark.
Context: ...as locally defined pure functions. ::warning Make sure not to reference any ...(UNLIKELY_OPENING_PUNCTUATION)
280-283: Consider adding TypeScript typesThe validation function example could benefit from TypeScript types for better type safety and documentation:
-function validateIdParam(route) { +function validateIdParam(route: RouteLocationNormalized): boolean { return route.params.id && !isNaN(Number(route.params.id)) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/2.guide/2.directory-structure/1.pages.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/2.guide/2.directory-structure/1.pages.md
[misspelling] ~270-~270: This word is normally spelled as one.
Context: ...t of the component. Therefore, the page meta object cannot reference the component. However...
(EN_COMPOUNDS_META_OBJECT)
[uncategorized] ~271-~271: Loose punctuation mark.
Context: ...as locally defined pure functions. ::warning Make sure not to reference any ...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~273-~273: Loose punctuation mark.
Context: .... This can lead to unexpected behavior. :: ```vue <script setup lang="ts"> impor...
(UNLIKELY_OPENING_PUNCTUATION)
🔇 Additional comments (2)
docs/2.guide/2.directory-structure/1.pages.md (2)
272-274: LGTM! Clear and important warning
The warning effectively communicates the limitations regarding reactive data and side effects in definePageMeta.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~273-~273: Loose punctuation mark.
Context: .... This can lead to unexpected behavior. :: ```vue <script setup lang="ts"> impor...
(UNLIKELY_OPENING_PUNCTUATION)
287-289: LGTM! Clear examples of correct and incorrect usage
The example effectively demonstrates:
- ✅ Correct usage of pure functions (
validateIdParam) - ✅ Correct usage of imported data (
someData) - ❌ Incorrect usage of refs (
title)
|
Amazing work - thank you so much ❤️ |
🔗 Linked issue
resolves #30197
📚 Description
This PR adds support for locally defined functions in
definePageMeta. A scope tracker was implemented to keep track of function's dependencies.This fixes a problem with esbuild defining helper functions when using the
vide.esbuild.keepNames = trueoption, for example:but also maybe a slightly more common use case, where the user provides a named function, regardless of the
esbuild.keepNamessetting:📝 TODO
Detect impure functionsShow a readable error message when an impure function is detectednuxt/packages/nuxt/src/core/plugins/composable-keys.tstoo