fix(unplugin): apply definePage path-param parser overrides#2699
Conversation
✅ Deploy Preview for vue-router canceled.
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughPath parameter overrides now accept null values to remove filename-derived parsers. The route override type is unified to ChangesPath Parameter Override System
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
commit: |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2699 +/- ##
==========================================
+ Coverage 86.13% 86.15% +0.01%
==========================================
Files 90 90
Lines 10173 10182 +9
Branches 2354 2360 +6
==========================================
+ Hits 8763 8772 +9
Misses 1399 1399
Partials 11 11 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/router/src/unplugin/core/tree.spec.ts (1)
1203-1258: ⚡ Quick winSolid coverage of the new override semantics.
Tests cleanly cover apply, override, partial override, and
null-removal. One gap worth considering: none of these tests exercise interaction with another override source (e.g., a convention override applied before a definePage override on the same param). Given the newnullsentinel relies onmergeRouteRecordOverridepreserving it through multi-source merges, a small test that registers two overrides for the same param viasetOverride(CONVENTION_OVERRIDE_NAME, ...)andsetOverride('@@edits', ...)would lock down that interaction.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/router/src/unplugin/core/tree.spec.ts` around lines 1203 - 1258, Add a test that verifies a definePage override with null survives when another override from conventions exists: create a PrefixTree and insert a node (use PrefixTree and node.insert), call node.setOverride(CONVENTION_OVERRIDE_NAME, { params: { path: { when: 'int' } } }) to simulate the convention override, then call node.setCustomRouteBlock(file, { params: { path: { when: null } } }) to apply the definePage override, and finally assert node.pathParams contains { paramName: 'when', parser: null }; this ensures mergeRouteRecordOverride preserves the null sentinel across multi-source merges.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/router/src/unplugin/core/treeNodeValue.ts`:
- Around line 429-437: The constructor signature was changed to a private
filenamePathParams which breaks external instantiation and drops the public
pathParams field; restore compatibility by reverting the third parameter to
public pathParams: TreePathParam[] (match original position/order) and if
internal code needs the filename-named field, assign it inside the constructor
(e.g. this.filenamePathParams = pathParams) or add a getter that returns
pathParams; update the constructor in the class containing constructor(...) and
ensure createTreeNodeValue still supplies the same arguments.
---
Nitpick comments:
In `@packages/router/src/unplugin/core/tree.spec.ts`:
- Around line 1203-1258: Add a test that verifies a definePage override with
null survives when another override from conventions exists: create a PrefixTree
and insert a node (use PrefixTree and node.insert), call
node.setOverride(CONVENTION_OVERRIDE_NAME, { params: { path: { when: 'int' } }
}) to simulate the convention override, then call node.setCustomRouteBlock(file,
{ params: { path: { when: null } } }) to apply the definePage override, and
finally assert node.pathParams contains { paramName: 'when', parser: null };
this ensures mergeRouteRecordOverride preserves the null sentinel across
multi-source merges.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2182abe6-070d-46a8-87b4-4c75043f62c4
📒 Files selected for processing (3)
packages/router/src/unplugin/core/customBlock.tspackages/router/src/unplugin/core/tree.spec.tspackages/router/src/unplugin/core/treeNodeValue.ts
| constructor( | ||
| rawSegment: string, | ||
| parent: TreeNodeValue | undefined, | ||
| public pathParams: TreePathParam[], | ||
| private filenamePathParams: TreePathParam[], | ||
| pathSegment: string, | ||
| subSegments: SubSegment[] | ||
| ) { | ||
| super(rawSegment, parent, pathSegment, subSegments) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find any other constructor calls or assignments to pathParams on TreeNodeValueParam
ast-grep --pattern 'new TreeNodeValueParam($$$)'
rg -nP '\.pathParams\s*=' --type=tsRepository: vuejs/router
Length of output: 566
🏁 Script executed:
#!/bin/bash
# Check if TreeNodeValueParam is exported from the package
echo "=== Checking for TreeNodeValueParam exports ==="
rg -n "export.*TreeNodeValueParam" --type=ts
echo -e "\n=== Checking what's in the main entry points ==="
git ls-files | grep -E "(index\.(ts|js|d\.ts)|package\.json)" | head -20
echo -e "\n=== Check if TreeNodeValueParam appears in type definitions ==="
rg -n "TreeNodeValueParam" --type=ts | head -30Repository: vuejs/router
Length of output: 1616
Constructor signature change — confirm no other call sites.
Switching the third positional parameter from public pathParams to private filenamePathParams is a breaking change for any external code constructing TreeNodeValueParam directly, since this class is exported as part of the public API. The only in-repo caller is createTreeNodeValue (line 615-621), and no other instantiation paths or .pathParams assignments exist in the repository. However, external consumers could be affected by the parameter order change and the field visibility change (now getter-only).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/router/src/unplugin/core/treeNodeValue.ts` around lines 429 - 437,
The constructor signature was changed to a private filenamePathParams which
breaks external instantiation and drops the public pathParams field; restore
compatibility by reverting the third parameter to public pathParams:
TreePathParam[] (match original position/order) and if internal code needs the
filename-named field, assign it inside the constructor (e.g.
this.filenamePathParams = pathParams) or add a getter that returns pathParams;
update the constructor in the class containing constructor(...) and ensure
createTreeNodeValue still supplies the same arguments.
Summary
definePage({ params: { path: { when: 'date' } } })now applies the parser, matching the filename convention[when=date].vue.nullin the override to remove a filename-based parser.TreeNodeValueParam.pathParamsis now a getter that overlays the leaf node'sparams.pathoverride on top of the filename-parsed params.Test plan
tree.spec.tsunderPath param parsers from definePage:[when]path param.[when=int]→date).nulloverride removes the filename parser.pnpm exec vitest run src/unplugin/).Summary by CodeRabbit
New Features
Tests