Conversation
… params for skipRouteOnParseError routes
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughExports Changes
Sequence Diagram(s)sequenceDiagram
participant Router as Router.buildLocation
participant Branch as buildRouteBranch
participant Route as Route.stringify
participant Interp as interpolatePath
participant Dest as DestinationResolver
Router->>Branch: request branch info (stringifiers) for target route
Branch-->>Router: route branch (stringifiers)
Router->>Route: apply stringify across branch -> prestringifiedParams
Route-->>Router: prestringifiedParams
Router->>Interp: interpolatePath(nextTo, prestringifiedParams)
Interp-->>Router: interpolatedNextTo
Router->>Dest: resolve destination with interpolatedNextTo
Dest-->>Router: resolved destination (fullPath)
alt destination matches trimmed target
Router->>Router: use interpolatedNextTo as nextPathname
else destination mismatch
Router->>Route: compute stringifiedParams fallback (no prestringified)
Route-->>Router: stringifiedParams
Router->>Interp: interpolatePath(nextTo, stringifiedParams)
Interp-->>Router: nextPathname
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
View your CI Pipeline Execution ↗ for commit a3c3a56
☁️ Nx Cloud last updated this comment at |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/router-core/src/router.ts`:
- Around line 1803-1807: The interpolatedNextTo is currently built from
nextParams (parsed values) but later code sometimes needs the stringified values
in prestringifiedParams; change the interpolation so that when
prestringifiedParams is provided you call interpolatePath with params:
prestringifiedParams (and same decoder) instead of nextParams, or alternatively
compute a second value (e.g., interpolatedPrestringifiedNextTo) by calling
interpolatePath({ path: nextTo, params: prestringifiedParams, decoder:
this.pathParamsDecoder }) and use that result wherever the code currently
branches on prestringifiedParams (including the return path and any route
matching that relies on interpolatedNextTo). Ensure the unique symbols involved
are interpolatePath, interpolatedNextTo (or new
interpolatedPrestringifiedNextTo), nextParams, and prestringifiedParams so the
correct interpolated pathname is returned and used for matching.
…-interpolate-stringified-params
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #6490
Warning
I'm not 100% convinced this is a good idea.
Problem & proposed solution:
Currently before we look for a branch match in the route tree, we
interpolatePathusingtoand the rawparamsprovided by the user. This bypasses theparams.stringifyoption.Up until now, this hasn't been an issue because all the path params always accept any string (we can imagine cases where this would in fact have been an issue, for example it the stringification adds a prefix, and then with the prefix it's actually another route that is matched, but such cases have seemingly never been reported).
However, with the introduction of
skipRouteOnParseError, we may now useparams.parsein the matching algorithm, andparams.parseexpects its inputs to have gone throughparams.stringify(see #6490). And if they didn't, this can result in incorrect branch matches.This PR proposes that we call
interpolatePathafter theparamshave gone throughparams.stringify. This is a little weird because to do this we have to "trust" that thetoparameter is correct, but then we still run the match algorithm, which could return a different branch (i.e. a differentto).Note
Maybe to cover edge-cases we could re-run
interpolatePathandgetMatchedRouteson the original raw params if the resulting matched branch isn't the same as the one we "naively inferred" throughbuildRouteBranch. But that would be quite the overhead.Possible issues:
toprops, and then it wouldn't match anything in theroutesByPathlookupto(intended behavior) but by then we will have stringified the params using the originalto(weird behavior?)encodeURIComponentthe params instead of going throughparams.stringify? I have a hard time believing we always know which route branch we're working onHowever, if you respect the types, and you don't have a crazy route setup that makes conflict likely, then this is pretty powerful (see original issue).
The alternatives are:
skipRouteOnParseError.params: trueandparams.stringifyskipRouteOnParseError.paramsthan the one used for "reversing the stringification"🤷
Summary by CodeRabbit
New Features
Improvements
Tests