fix(router-core): review follow-ups for Link stack - #8410
schiller-manuel merged 6 commits into
Conversation
The router-core Link/path benchmarks and the react-link-performance project are never part of the CodSpeed matrix or the client-nav/SSR aggregate projects, so the env-var gate only added friction. Rely on standard vitest name filtering (-t) to narrow runs instead. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Splat interpolation used a falsy check, so _splat: 0 was omitted like a missing param while $id: 0 interpolated to '/0'. Only undefined, null, and '' now omit a splat segment, matching how other path params are stringified. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The matcher only uses route ids as routesById keys and for the pathless '_' check, so unrelated ids worked but never occur in a real router. Align the fixtures with the sibling test file's convention where each id mirrors its fullPath. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (7)
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review. 📝 WalkthroughWalkthroughThe change preserves ChangesPath interpolation behavior
Benchmark execution defaults
Link props cleanup
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to The interpolation behavior and benchmark defaults are consistently implemented and covered, with no identified issue that should block merging. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
|
View your CI Pipeline Execution ↗ for commit 0b85fd8
☁️ Nx Cloud last updated this comment at |
🚀 Changeset Version Preview7 package(s) bumped directly, 22 bumped as dependents. 🟩 Patch bumps
|
Bundle Size Benchmarks
The following scenarios have bundle-size changes compared with the baseline:
Current gzip tracks all emitted client JS chunks. Initial gzip tracks only the entry/import graph. Trend sparkline is historical current gzip ending with this PR measurement; lower is better. |
Spread `cond && { value }` instead of `cond ? { value } : undefined`,
matching the neighbouring `active && STATIC_ACTIVE_ATTRIBUTES` spread.
Falsy operands spread to nothing, so the emitted props are unchanged.
Measured with the bundle-size benchmark on solid-router.full:
gzip 39205 -> 39198 (-7), raw -12, brotli -33.
Covered by the existing `six-adults-open` changeset.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…ests The `interpolatePath (server: $server)` block never read its `server` argument since interpolation stopped taking a server flag, so it ran every case twice under a misleading label. The remaining diff is re-indentation; review with `git show -w`. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
`interpolateTestPath` only survived as a compatibility layer from the
segment-reuse refactor: it emulated the removed
`(path, params, decoder, usedParams, keys, metadata)` signature by parsing
the template, re-implementing the param-name loop and folding
`hasMissingPathParams` into a metadata flag.
Call the real functions instead. Tests now parse templates with
`parseSegments(false, { fullPath }, 0)` exactly like `buildLocation` does
for templates without a processed route, pass the segments to
`interpolatePath`, and read names and availability from `getParamNames`
and `hasMissingPathParams`. The prepared-route tests compare route-owned
segments against that standalone parse directly, and the interpolation
bench derives its reference values the same way.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
* chore(benchmarks): remove TSR_LINK_PERF opt-in flag
The router-core Link/path benchmarks and the react-link-performance
project are never part of the CodSpeed matrix or the client-nav/SSR
aggregate projects, so the env-var gate only added friction. Rely on
standard vitest name filtering (-t) to narrow runs instead.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
* fix(router-core): treat 0 and false as provided _splat values
Splat interpolation used a falsy check, so _splat: 0 was omitted like a
missing param while $id: 0 interpolated to '/0'. Only undefined, null,
and '' now omit a splat segment, matching how other path params are
stringified.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
* test(router-core): use path-derived ids in original-param-names fixtures
The matcher only uses route ids as routesById keys and for the pathless
'_' check, so unrelated ids worked but never occur in a real router.
Align the fixtures with the sibling test file's convention where each
id mirrors its fullPath.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
* perf(solid-router): shorten conditional Link class and style spreads
Spread `cond && { value }` instead of `cond ? { value } : undefined`,
matching the neighbouring `active && STATIC_ACTIVE_ATTRIBUTES` spread.
Falsy operands spread to nothing, so the emitted props are unchanged.
Measured with the bundle-size benchmark on solid-router.full:
gzip 39205 -> 39198 (-7), raw -12, brotli -33.
Covered by the existing `six-adults-open` changeset.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
* test(router-core): drop the unused server axis from interpolatePath tests
The `interpolatePath (server: $server)` block never read its `server`
argument since interpolation stopped taking a server flag, so it ran every
case twice under a misleading label. The remaining diff is re-indentation;
review with `git show -w`.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
* test(router-core): interpolate paths through the production API in tests
`interpolateTestPath` only survived as a compatibility layer from the
segment-reuse refactor: it emulated the removed
`(path, params, decoder, usedParams, keys, metadata)` signature by parsing
the template, re-implementing the param-name loop and folding
`hasMissingPathParams` into a metadata flag.
Call the real functions instead. Tests now parse templates with
`parseSegments(false, { fullPath }, 0)` exactly like `buildLocation` does
for templates without a processed route, pass the segments to
`interpolatePath`, and read names and availability from `getParamNames`
and `hasMissingPathParams`. The prepared-route tests compare route-owned
segments against that standalone parse directly, and the interpolation
bench derives its reference values the same way.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
---------
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Follow-ups from review on #8327, #8318, #8319 and #8325, stacked on top of #8406 (stack #8346).
TSR_LINK_PERFopt-in flag from the router-core Link/path benchmarks and thereact-link-performanceproject; rely on standard vitest-tname filtering instead. These benchmarks were never part of the CodSpeed matrix or the client-nav/SSR aggregate projects.0andfalseas provided_splatvalues when interpolating paths. Onlyundefined,null, and''omit a splat segment, matching how$id: 0already interpolates to/0. Includes a changeset.original-param-namesmatcher fixtures, matching the convention innew-process-route-tree.test.ts.Link: spreadcond && { value }for the conditionalclass/styleprops (perf(router): optimize Link state props across frameworks #8318 review). Measured onsolid-router.full: gzip −7, raw −12, brotli −33. Covered by the existingsix-adults-openchangeset.parseSegments,interpolatePath,getParamNamesandhasMissingPathParamsdirectly instead of theinterpolateTestPathcompatibility shim, and drop theinterpolatePath (server: …)axis that no longer read its argument.Review notes for threads that needed no code change here:
blockedLink ? … : STATIC_EMPTY_OBJECTspreads): superseded —applyLinkStatenow applies state props withObject.assignand pinshref/disabled/targetafterwards.metadata.isMissingParamsinternal key): superseded —interpolatePathno longer takes a metadata object;hasMissingPathParamsreturns a boolean.normalizeProtocolRelativefor interpolated paths):buildLocationwraps both the interpolated and the literaltoinnormalizeProtocolRelative(decodePath(…));history-normalization.test.ts› "normalizes a generated splat destination before output rewriting" covers/$+{ _splat: '/other.example/path' }→/other.example/path.Summary by CodeRabbit
Bug Fixes
0andfalse.null,undefined, or empty strings.Documentation
Performance