types(router): add | undefined to optional properties for exactOptionalPropertyTypes#2634
Conversation
✅ Deploy Preview for vue-router canceled.
|
|
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:
📝 WalkthroughWalkthroughThe changes make optional properties throughout the router package explicitly allow Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 #2634 +/- ##
==========================================
- Coverage 85.58% 85.57% -0.01%
==========================================
Files 86 86
Lines 9960 9958 -2
Branches 2285 2285
==========================================
- Hits 8524 8522 -2
Misses 1423 1423
Partials 13 13 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/router/src/unplugin/core/treeNodeValue.ts (1)
41-44: Redundant field re-declarations — safe to remove.
defaultandrequiredare already declared inCustomRouteBlockQueryParamOptionswith identical types (string | undefinedandboolean | undefinedrespectively). SinceRouteRecordOverrideQueryParamOptionsdoes not narrow or widen them, the re-declarations here are purely redundant.♻️ Proposed cleanup
export interface RouteRecordOverrideQueryParamOptions extends CustomRouteBlockQueryParamOptions { - default?: string | undefined - required?: boolean | undefined }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/router/src/unplugin/core/treeNodeValue.ts` around lines 41 - 44, RouteRecordOverrideQueryParamOptions redundantly re-declares default and required which are already defined in CustomRouteBlockQueryParamOptions; remove the duplicate properties from the RouteRecordOverrideQueryParamOptions interface so it simply extends CustomRouteBlockQueryParamOptions without re-declaring default or required (locate the RouteRecordOverrideQueryParamOptions interface in treeNodeValue.ts and delete the two property lines).packages/router/src/matcher/index.ts (1)
377-400:toPathParamsduplicatespickParamslogic — consider consolidation.
toPathParamsis functionally equivalent to callingpickParams(params, Object.keys(params)). Both filterundefinedfromMatcherLocation['params']intoPathParams. The only structural difference is thatpickParamspre-checkskey in params, which is equivalent tofor...inon a plain object like the one produced byassign({}, ...).♻️ Optional consolidation
-function toPathParams(params: MatcherLocation['params']): PathParams { - const newParams: PathParams = {} - for (const key in params) { - const value = params[key] - if (value !== undefined) newParams[key] = value - } - return newParams -}Then at the call site (line 331):
-params = toPathParams(assign({}, currentLocation.params, location.params)) +params = pickParams( + assign({}, currentLocation.params, location.params), + Object.keys(assign({}, currentLocation.params, location.params)) +)Alternatively, make
keysoptional inpickParamsand default toObject.keys(params)— whichever is cleaner for the codebase.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/router/src/matcher/index.ts` around lines 377 - 400, toPathParams duplicates pickParams logic; consolidate by removing the duplicate: either delete toPathParams and replace its callers with pickParams(params, Object.keys(params)), or change pickParams(params, keys) to accept an optional keys parameter (keys?: string[]) and default to Object.keys(params) so callers can simply call pickParams(params). Update all references to toPathParams to use pickParams and keep the behavior of filtering out undefined values intact (preserve the existing check for value !== undefined).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/router/src/experimental/data-loaders/auto-exports.ts`:
- Around line 79-81: The `@ts-expect-error` above the id: transformFilter
assignment can become a CI-breaking error if upstream types converge; update the
comment block immediately above the id: transformFilter line to keep the
existing explanation but append a clear FIXME/TODO that documents why the
directive is present, the expected condition for removal (e.g., when
vite/unplugin StringFilter types align), and optionally a link or issue
reference to track removal; ensure the comment mentions transformFilter and that
the `@ts-expect-error` should be removed once the type gap is closed so future
maintainers know to revisit it.
---
Nitpick comments:
In `@packages/router/src/matcher/index.ts`:
- Around line 377-400: toPathParams duplicates pickParams logic; consolidate by
removing the duplicate: either delete toPathParams and replace its callers with
pickParams(params, Object.keys(params)), or change pickParams(params, keys) to
accept an optional keys parameter (keys?: string[]) and default to
Object.keys(params) so callers can simply call pickParams(params). Update all
references to toPathParams to use pickParams and keep the behavior of filtering
out undefined values intact (preserve the existing check for value !==
undefined).
In `@packages/router/src/unplugin/core/treeNodeValue.ts`:
- Around line 41-44: RouteRecordOverrideQueryParamOptions redundantly
re-declares default and required which are already defined in
CustomRouteBlockQueryParamOptions; remove the duplicate properties from the
RouteRecordOverrideQueryParamOptions interface so it simply extends
CustomRouteBlockQueryParamOptions without re-declaring default or required
(locate the RouteRecordOverrideQueryParamOptions interface in treeNodeValue.ts
and delete the two property lines).
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
packages/router/__tests__/guards/onBeforeRouteUpdate.spec.tspackages/router/__tests__/matcher/resolve.spec.tspackages/router/__tests__/utils.tspackages/router/src/RouterLink.tspackages/router/src/experimental/data-loaders/auto-exports.tspackages/router/src/experimental/data-loaders/defineColadaLoader.tspackages/router/src/experimental/data-loaders/utils.tspackages/router/src/experimental/route-resolver/matchers/matcher-pattern.tspackages/router/src/experimental/route-resolver/resolver-abstract.tspackages/router/src/experimental/route-resolver/resolver-fixed.tspackages/router/src/experimental/router.tspackages/router/src/location.tspackages/router/src/matcher/index.tspackages/router/src/matcher/pathParserRanker.tspackages/router/src/scrollBehavior.tspackages/router/src/typed-routes/route-location.tspackages/router/src/types/index.tspackages/router/src/unplugin/core/RoutesFolderWatcher.tspackages/router/src/unplugin/core/context.tspackages/router/src/unplugin/core/customBlock.tspackages/router/src/unplugin/core/definePage.tspackages/router/src/unplugin/core/tree.tspackages/router/src/unplugin/core/treeNodeValue.tspackages/router/src/unplugin/core/utils.tspackages/router/src/unplugin/options.tspackages/router/tsconfig.json
|
Thanks a lot! I will have to take a proper look, but ideally there should be no runtime changes. The |
YevheniiKotyrlo
force-pushed
the
types/exact-optional-property-types
branch
2 times, most recently
from
1133bcf to
d288538
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/router/src/experimental/route-resolver/resolver-fixed.ts (1)
46-46:⚠️ Potential issue | 🟡 Minor
queryis missing| undefined— inconsistent with the other updated properties.
name,path, andhashall received| undefinedto satisfyexactOptionalPropertyTypes: true, butquerywas not updated. A consumer with that flag enabled who writesrecord.query = undefinedwould still get a type error, and subtypes overriding this property toMatcherPatternQuery[] | undefinedwould fail to extend the base interface.🛠️ Proposed fix
- query?: MatcherPatternQuery[] + query?: MatcherPatternQuery[] | undefined🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/router/src/experimental/route-resolver/resolver-fixed.ts` at line 46, The optional property "query" is missing an explicit "| undefined" causing inconsistency with "name", "path", and "hash" under exactOptionalPropertyTypes; update the property declaration for query (the one currently typed as query?: MatcherPatternQuery[]) to include undefined (query?: MatcherPatternQuery[] | undefined) so consumers can assign undefined and subtypes with MatcherPatternQuery[] | undefined will correctly extend the base type.packages/router/src/types/index.ts (1)
284-297:⚠️ Potential issue | 🟡 Minor
component?: RawRouteComponent | null— should also allowundefinedfor consistency.In the
exactOptionalPropertyTypeschanges (commit d288538), this field was expanded to| null, but it's missing| undefined. Other optional properties in this file use only| undefined(e.g.,force,state,redirect), while the parallelcomponentsfield inRouteRecordMultipleViewsWithChildrenexplicitly includes both| null | undefined. SincenormalizeRouteRecordline 429 treats both null and undefined identically via the falsy checkrecord.component && { ... }, this should be:component?: RawRouteComponent | null | undefined🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/router/src/types/index.ts` around lines 284 - 297, The component property in RouteRecordSingleViewWithChildren currently allows RawRouteComponent | null but not undefined; update the type to component?: RawRouteComponent | null | undefined to match other optional fields and the parallel components field in RouteRecordMultipleViewsWithChildren, and ensure callers like normalizeRouteRecord treat null/undefined the same (no behavioral change required beyond the type widening).
♻️ Duplicate comments (1)
packages/router/src/experimental/data-loaders/auto-exports.ts (1)
79-81: Previously reviewed and resolved.The
@ts-expect-errordirective and its accompanying explanation were discussed in a prior review. The approach is intentional: the directive is self-enforcing — if upstreamStringFiltertypes converge, the build will fail and signal its removal. No further action needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/router/src/experimental/data-loaders/auto-exports.ts` around lines 79 - 81, No change required: retain the `@ts-expect-error` and explanatory comment above the id: transformFilter assignment in auto-exports.ts — the directive is intentional and self-enforcing so leave the transformFilter binding and its comment as-is (it will surface a build failure if upstream StringFilter types converge and then can be removed).
🧹 Nitpick comments (1)
packages/router/src/experimental/route-resolver/matchers/matcher-pattern.ts (1)
233-239: Type widening forparseris correct; consider a type alias to reduce verbosity.The explicit
| undefinedis required byexactOptionalPropertyTypessinceMatcherPatternPathDynamic_ParamOptions's first tuple element isparser?: ParamParser<...>. The usage at lines 252 and 272 already guards withparser?.set, so undefined is handled safely.The annotation is quite unwieldy. A local type alias could improve readability:
♻️ Optional: extract a local type alias
+ type ParserType = + | (TParamsOptions & Record<string, MatcherPatternPathDynamic_ParamOptions<any, any>>)[keyof TParamsOptions][0] + | undefined + build( params: Simplify<ExtractLocationParamTypeFromOptions<TParamsOptions>> ): string { let paramIndex = 0 let paramName: keyof TParamsOptions - let parser: - | (TParamsOptions & - Record< - string, - MatcherPatternPathDynamic_ParamOptions<any, any> - >)[keyof TParamsOptions][0] - | undefined + let parser: ParserType🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/router/src/experimental/route-resolver/matchers/matcher-pattern.ts` around lines 233 - 239, The current explicit type for the local variable parser (the union over TParamsOptions & Record<...>[keyof TParamsOptions][0] | undefined) is correct and must keep the | undefined due to exactOptionalPropertyTypes and MatcherPatternPathDynamic_ParamOptions' optional parser, but it's verbose: introduce a local type alias (e.g., type ParamOption = (TParamsOptions & Record<string, MatcherPatternPathDynamic_ParamOptions<any, any>>)[keyof TParamsOptions][0] | undefined) and annotate parser with that alias, leaving the existing parser?.set uses unchanged; this keeps correctness while improving readability.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/router/src/types/index.ts`:
- Around line 42-45: The change to RouteParamsGeneric intentionally widens the
value type to include undefined to support exactOptionalPropertyTypes (see
RouteParamsGeneric and exactOptionalPropertyTypes.test-d.ts); add a clear
changelog entry and release notes explaining this breaking change, describe
migration guidance for consumers (e.g., show patterns: filter out undefined
values, use type narrowing or non-null assertions before calling string methods,
or map only defined values), and update any docs/tests that assumed
non-undefined param values to demonstrate the correct handling for optional
repeatable params like [[files]]+.
---
Outside diff comments:
In `@packages/router/src/experimental/route-resolver/resolver-fixed.ts`:
- Line 46: The optional property "query" is missing an explicit "| undefined"
causing inconsistency with "name", "path", and "hash" under
exactOptionalPropertyTypes; update the property declaration for query (the one
currently typed as query?: MatcherPatternQuery[]) to include undefined (query?:
MatcherPatternQuery[] | undefined) so consumers can assign undefined and
subtypes with MatcherPatternQuery[] | undefined will correctly extend the base
type.
In `@packages/router/src/types/index.ts`:
- Around line 284-297: The component property in
RouteRecordSingleViewWithChildren currently allows RawRouteComponent | null but
not undefined; update the type to component?: RawRouteComponent | null |
undefined to match other optional fields and the parallel components field in
RouteRecordMultipleViewsWithChildren, and ensure callers like
normalizeRouteRecord treat null/undefined the same (no behavioral change
required beyond the type widening).
---
Duplicate comments:
In `@packages/router/src/experimental/data-loaders/auto-exports.ts`:
- Around line 79-81: No change required: retain the `@ts-expect-error` and
explanatory comment above the id: transformFilter assignment in auto-exports.ts
— the directive is intentional and self-enforcing so leave the transformFilter
binding and its comment as-is (it will surface a build failure if upstream
StringFilter types converge and then can be removed).
---
Nitpick comments:
In `@packages/router/src/experimental/route-resolver/matchers/matcher-pattern.ts`:
- Around line 233-239: The current explicit type for the local variable parser
(the union over TParamsOptions & Record<...>[keyof TParamsOptions][0] |
undefined) is correct and must keep the | undefined due to
exactOptionalPropertyTypes and MatcherPatternPathDynamic_ParamOptions' optional
parser, but it's verbose: introduce a local type alias (e.g., type ParamOption =
(TParamsOptions & Record<string, MatcherPatternPathDynamic_ParamOptions<any,
any>>)[keyof TParamsOptions][0] | undefined) and annotate parser with that
alias, leaving the existing parser?.set uses unchanged; this keeps correctness
while improving readability.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
packages/router/__tests__/guards/extractComponentsGuards.spec.tspackages/router/__tests__/guards/onBeforeRouteUpdate.spec.tspackages/router/__tests__/matcher/resolve.spec.tspackages/router/__tests__/utils.tspackages/router/src/RouterLink.tspackages/router/src/exactOptionalPropertyTypes.test-d.tspackages/router/src/experimental/data-loaders/auto-exports.tspackages/router/src/experimental/data-loaders/defineColadaLoader.tspackages/router/src/experimental/data-loaders/utils.tspackages/router/src/experimental/route-resolver/matchers/matcher-pattern.tspackages/router/src/experimental/route-resolver/resolver-fixed.tspackages/router/src/location.tspackages/router/src/matcher/index.tspackages/router/src/matcher/pathParserRanker.tspackages/router/src/scrollBehavior.tspackages/router/src/typed-routes/route-location.tspackages/router/src/types/index.tspackages/router/src/unplugin/core/customBlock.tspackages/router/src/unplugin/core/definePage.tspackages/router/src/unplugin/core/treeNodeValue.tspackages/router/src/unplugin/options.tspackages/router/tsconfig.json
💤 Files with no reviewable changes (1)
- packages/router/tests/guards/extractComponentsGuards.spec.ts
✅ Files skipped from review due to trivial changes (1)
- packages/router/src/exactOptionalPropertyTypes.test-d.ts
🚧 Files skipped from review as they are similar to previous changes (10)
- packages/router/tsconfig.json
- packages/router/src/matcher/pathParserRanker.ts
- packages/router/tests/utils.ts
- packages/router/src/unplugin/options.ts
- packages/router/tests/guards/onBeforeRouteUpdate.spec.ts
- packages/router/src/experimental/data-loaders/utils.ts
- packages/router/src/typed-routes/route-location.ts
- packages/router/src/unplugin/core/customBlock.ts
- packages/router/src/scrollBehavior.ts
- packages/router/src/unplugin/core/treeNodeValue.ts
YevheniiKotyrlo
force-pushed
the
types/exact-optional-property-types
branch
from
d288538 to
f7afa57
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/router/src/matcher/index.ts (1)
377-400: Consider consolidating the undefined-filtering logic.Both
pickParamsandtoPathParamshave similar logic for filtering outundefinedvalues. While the PR author noted these serve different intents (key selection vs full conversion), the filtering logic is duplicated. An optional refactor could extract the filtering to a shared utility:function filterUndefined<T>(obj: Record<string, T | undefined>): Record<string, T> { const result: Record<string, T> = {} for (const key in obj) { const value = obj[key] if (value !== undefined) result[key] = value } return result }However, given the author's stated intent to keep them separate for call-site clarity and the small code footprint, this is acceptable as-is.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/router/src/matcher/index.ts` around lines 377 - 400, Extract the duplicated undefined-filtering loop into a shared utility (e.g., filterUndefined) and use it inside both pickParams and toPathParams: implement a function that accepts a Record<string, T | undefined> and returns a Record<string, T> with keys whose values are not undefined, then have pickParams call filterUndefined on an object built from the provided keys and params (or map the selected keys to an object first) and have toPathParams simply return filterUndefined(params); update the references to use filterUndefined while preserving the original function names and behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/router/src/matcher/index.ts`:
- Around line 377-400: Extract the duplicated undefined-filtering loop into a
shared utility (e.g., filterUndefined) and use it inside both pickParams and
toPathParams: implement a function that accepts a Record<string, T | undefined>
and returns a Record<string, T> with keys whose values are not undefined, then
have pickParams call filterUndefined on an object built from the provided keys
and params (or map the selected keys to an object first) and have toPathParams
simply return filterUndefined(params); update the references to use
filterUndefined while preserving the original function names and behavior.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
packages/router/__tests__/guards/extractComponentsGuards.spec.tspackages/router/__tests__/guards/onBeforeRouteUpdate.spec.tspackages/router/__tests__/matcher/resolve.spec.tspackages/router/__tests__/utils.tspackages/router/src/RouterLink.tspackages/router/src/exactOptionalPropertyTypes.test-d.tspackages/router/src/experimental/data-loaders/auto-exports.tspackages/router/src/experimental/data-loaders/defineColadaLoader.tspackages/router/src/experimental/data-loaders/utils.tspackages/router/src/experimental/route-resolver/matchers/matcher-pattern.tspackages/router/src/experimental/route-resolver/resolver-fixed.tspackages/router/src/location.tspackages/router/src/matcher/index.tspackages/router/src/matcher/pathParserRanker.tspackages/router/src/scrollBehavior.tspackages/router/src/typed-routes/route-location.tspackages/router/src/types/index.tspackages/router/src/unplugin/core/customBlock.tspackages/router/src/unplugin/core/definePage.tspackages/router/src/unplugin/core/treeNodeValue.tspackages/router/src/unplugin/options.tspackages/router/tsconfig.json
💤 Files with no reviewable changes (1)
- packages/router/tests/guards/extractComponentsGuards.spec.ts
🚧 Files skipped from review as they are similar to previous changes (10)
- packages/router/src/experimental/data-loaders/auto-exports.ts
- packages/router/src/typed-routes/route-location.ts
- packages/router/src/matcher/pathParserRanker.ts
- packages/router/tests/matcher/resolve.spec.ts
- packages/router/tests/utils.ts
- packages/router/src/unplugin/core/definePage.ts
- packages/router/src/types/index.ts
- packages/router/src/unplugin/options.ts
- packages/router/tsconfig.json
- packages/router/src/unplugin/core/treeNodeValue.ts
posva
left a comment
posva
left a comment
There was a problem hiding this comment.
A few other places where the runtime is still affected
| const spy = vi.fn() | ||
| const Component = defineComponent({ | ||
| name, | ||
| ...(name != null ? { name } : {}), |
There was a problem hiding this comment.
Shouldn't this work in Vue?
In this case, I think it's better to have the fallback
| ...(name != null ? { name } : {}), | |
| name: name || 'no-name', |
packages/router/src/matcher/index.ts
Outdated
| return newParams | ||
| } | ||
|
|
||
| function toPathParams(params: MatcherLocation['params']): PathParams { |
There was a problem hiding this comment.
This change to the matcher shouldn't be runtime
|
|
||
| return { | ||
| behavior: offset.behavior, | ||
| ...(offset.behavior != null && { behavior: offset.behavior }), |
There was a problem hiding this comment.
this one shouldn't be needed either
packages/router/src/RouterLink.ts
Outdated
There was a problem hiding this comment.
shouldn't be needed either, the type should be stricter
YevheniiKotyrlo
force-pushed
the
types/exact-optional-property-types
branch
from
f7afa57 to
e65e258
Compare
YevheniiKotyrlo
changed the title
YevheniiKotyrlo
force-pushed
the
types/exact-optional-property-types
branch
from
e65e258 to
c1e16a0
Compare
|
Thanks for the detailed review! Rebased onto latest main and squashed into a single commit. Comment 1 —
|
2 participants
Summary
Add
| undefinedto all optional interface properties across published and internal types. This is a no-op for the default TypeScript configuration but fixes TS2375 errors for consumers withexactOptionalPropertyTypes: true.Follows the same approach as vuejs/core#12771 which was merged for
@vue/runtime-dom.Zero runtime changes. Zero type assertions.
Changes
| undefinedto optional properties in published interfaces (RouteLocationOptions,_RouteRecordBase,PathParserOptions, etc.)| undefinedto optional properties in internal/experimental types (unplugin, data-loaders, route-resolver)TrackedRoute.paramstype fromPartial<LocationQuery>toPartial<RouteParamsGeneric>(bug: params are not query values)exactOptionalPropertyTypes.test-d.tswith type-level testsNot in this PR (proposed as separate follow-up)
Enabling
exactOptionalPropertyTypesin the tsconfig also requiresRouteParamsGenericto include| undefinedfor optional repeatable params (ParamValueZeroOrMore<false> = string[] | undefined). This cascades into the matcher and RouterLink — the proper type-level fix is narrowingMatcherLocation.paramsfromRouteParamsGenerictoPathParamsafter resolution (~8-10 files). I'll propose that as a separate PR.Not a breaking change
exactOptionalPropertyTypes: false(default)T | undefinedis identical toTfor optional propertiesexactOptionalPropertyTypes: trueVerification
build✓test:types✓ (0 errors)test:unit✓ (1490 passed, 19 skipped, 11 todo)