fix: support arbitrary nesting depth in nestedPath helper - #698
Open
SaqlainR55 wants to merge 1 commit into
Open
fix: support arbitrary nesting depth in nestedPath helper#698SaqlainR55 wants to merge 1 commit into
SaqlainR55 wants to merge 1 commit into
Conversation
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
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.
Please check if the PR fulfills these requirements
What kind of change does this PR introduce?
Bug fix + small feature addition. The PR addresses three related issues in the
nestedPathhelper atpackages/webapp-libs/webapp-core/src/utils/path.ts, all surfaced when using the helper with route configurations nested 3+ levels deep.What is the current behavior?
1.
mapRootdoes not recurse past 2 levels of nesting.The helper claims to prefix nested route configs with the root, but the implementation of
mapRootonly iterates string-valued keys at the current level. When it encounters a child object (the result of an innernestedPathcall), it spreads the object through unchanged instead of recursing into it. As a result, the outermost prefix is silently dropped for any route nested 3 or more levels deep.Reproduction:
2. No helper for parent route mounts.
When mounting a nested route group as a parent
<Route>inside a child<Routes>tree, React Router requires a path of the form'segment/*'— the local segment with a wildcard suffix, no parent prefix (since React Router composes parent + child paths automatically).The helper exposes no method that returns this.
getRelativeUrl(key)returns the original input (the nested object for nested keys, or a leaf string),paths.indexreturns the absolute path with the parent prefix already baked in. Neither is suitable for a parent mount.Consumers work around this by hardcoding strings like
<Route path="customers/*" element={<Customers />} />. These hardcoded strings drift when the config is later renamed.3.
getRelativeUrlreturns a union of all value types.The current signature
(route: keyof T) => nestedRoutes[route]collapses to return typeT[keyof T]— the union of every value type in the config. For configs with mixed-value children (some string leaves, some nested objects), TypeScript widens tostring | NestedObject. Calling sites that pass the result to APIs expectingstring(e.g. React Router's<Route path>) reject the value and require a cast.What is the new behavior?
1.
mapRootnow recurses. Anelse ifbranch handles non-null, non-array object values by callingmapRoot(root, value)on them. The recursion is depth-agnostic; the same example above now produces'a/b/c/'correctly. Thevalue !== null && !Array.isArray(value)guard is defensive —nulland arrays both report as'object'fromtypeofand would corrupt the result if recursed into.2. New
getMountPath()method. Returns${root}/*for the currentnestedPathinvocation — the local segment plus wildcard, suitable for parent route mounts. Consumers can now write:The JSDoc has been extended with an
@exampleblock documenting when to usegetMountPath()(parent mount of a nested route group) versusgetRelativeUrl(key)(leaf route mapping directly to a component).3.
getRelativeUrlis generic over the key. Signature changes from(route: keyof T) => nestedRoutes[route]to<K extends keyof T>(route: K): T[K] => nestedRoutes[route]. TypeScript now narrows the return type per call:getRelativeUrl('cart')returnsstringwhencartis a string in the config, and the nested object type when the key points to a nested config. No more union; no more casts at the call site.Does this PR introduce a breaking change?
No.
mapRootrecursion fix only changes behavior for configs nested 3+ levels deep that were previously broken — no test or consumer in the repository depends on the broken behavior. The two existing 2-level tests continue to pass unchanged.getMountPath()is a new method; no existing call site is affected.getRelativeUrltype change is a strict improvement — every previously valid call resolves to a return type that's a subtype of the old union, so existing code continues to type-check (and additionally compiles in places that previously required casts).Other information
Tests added (in
path.spec.ts):getRelativeUrl,getLocalePath) on deeply nested objectsgetMountPath()at simple casegetMountPath()at multiple depthsgetMountPath()+getRelativeUrl()usage matching the JSDoc exampleThe two existing
toEqualshape assertions are updated to includegetMountPath: expect.any(Function)since the helper now returns this method on every nested config object.Discovery context: All three issues surfaced together while migrating a downstream app to a route configuration nested 3 levels deep (
admin → payments → customers → leaves). Each issue blocked the migration on its own: the recursion bug produced wrong URLs at runtime, the missing mount-path helper meant we had to choose between hardcoded literals or invalid casts, and the loosegetRelativeUrltyping made even the workaround fail TypeScript compilation. Bundling the three fixes here since they're tightly related and reviewing them together gives full context.