Skip to content

perf: optimize resolveParentParams lookups#718

Merged
james-elicx merged 5 commits intocloudflare:mainfrom
NathanDrake2406:perf/resolve-parent-params
Mar 30, 2026
Merged

perf: optimize resolveParentParams lookups#718
james-elicx merged 5 commits intocloudflare:mainfrom
NathanDrake2406:perf/resolve-parent-params

Conversation

@NathanDrake2406
Copy link
Copy Markdown
Contributor

@NathanDrake2406 NathanDrake2406 commented Mar 30, 2026

Summary

  • Map-based route index for O(1) parent route lookups (was O(N) linear scan via allRoutes.find())
  • Reuse pre-computed patternParts from AppRoute instead of re-splitting the pattern string on every call
  • Pre-compute last dynamic segment index via single reverse scan (was per-iteration slice().some() allocation)
  • Incremental prefix string building instead of slice(0, i+1).join("/") per dynamic segment
  • Remove dead ParentSegment.params field (allocated but never read)
  • Export StaticParamsMap type for reuse across production code and tests

These are in-memory micro-optimizations (~1ms savings on a 500-route site). The expensive HTTP fetches are already deduplicated by staticParamsCache. The wins are small but the code is cleaner — fewer allocations, O(1) lookups, and no dead fields.

Partially addresses #563 (ticks the resolveParentParams lookups checkbox).

Test plan

  • 9 direct unit tests for resolveParentParams covering: no parents, missing pagePath, missing generateStaticParams, single parent, two-level nesting, static segments between dynamic parents, fully static routes, single-segment dynamic routes, catch-all child segments
  • Full tests/prerender.test.ts integration suite passes (including nested /shop/[category]/[item] top-down params)
  • CI: Check, Vitest, Playwright E2E

- Use Map<pattern, route> index for O(1) route lookups (was O(N) linear scan)
- Use pre-computed patternParts instead of re-splitting pattern string
- Pre-compute last dynamic segment index (was per-iteration slice+some)
Cover fully static routes, single-segment dynamic routes, and
catch-all child segments.
- Remove dead ParentSegment.params field (allocated but never read)
- Replace slice+join with incremental prefix string building
- Export StaticParamsMap type so tests derive from source
- Improve comment to explain WHY last segment is excluded
@pkg-pr-new
Copy link
Copy Markdown

pkg-pr-new bot commented Mar 30, 2026

Open in StackBlitz

npm i https://pkg.pr.new/vinext@718

commit: 73a246f

@james-elicx
Copy link
Copy Markdown
Collaborator

/bigbonk review

Copy link
Copy Markdown
Contributor

@ask-bonk ask-bonk bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clean refactor — the logic is correct, the O(1) Map lookup is a nice improvement, and the dead ParentSegment.params field removal is welcome. Tests are thorough and well-structured. Two minor comments below.

@ask-bonk
Copy link
Copy Markdown
Contributor

ask-bonk bot commented Mar 30, 2026

Review submitted successfully. Here's a summary of what I reviewed and commented on:

Overall assessment: The PR is a clean, correct refactor. The logic transformation from O(N) allRoutes.find() to O(1) routeIndex.get() is sound, the incremental prefix string building is equivalent to the old slice/join approach, and the dead ParentSegment.params field removal eliminates an unused allocation. The 9 unit tests provide solid coverage of the key edge cases.

Two minor comments posted:

  1. Detached JSDoc (prerender.ts:255): The StaticParamsMap type was inserted between the existing resolveParentParams JSDoc and the function itself, so the doc comment now reads as if it documents the type alias rather than the function. Suggested reordering.

  2. Line swap suggestion (prerender.ts:291-292): prefixPattern += "/" + patternParts[i] and const part = patternParts[i] could be swapped so part is reused for both operations, avoiding the redundant array index. Cosmetic only.

github run

- Move StaticParamsMap type above JSDoc block so the function comment
  sits directly above resolveParentParams (was erroneously separating them)
- Declare 'part' before building prefixPattern so it's used for both the
  prefix string concatenation and the startsWith check, avoiding redundant
  array indexing
@james-elicx
Copy link
Copy Markdown
Collaborator

/bigbonk review

Copy link
Copy Markdown
Contributor

@ask-bonk ask-bonk bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clean, correct refactor. The O(1) Map lookup, incremental prefix building, and dead field removal are all straightforward wins. The new unit tests are thorough and cover the right edge cases.

CI is green (unit tests pass, integration tests pending at time of review but the relevant Vitest (unit) shard passed).

The two nits from the previous review (detached JSDoc, line swap for part reuse) are worth addressing but not blocking. One additional minor observation below.

params: parts
.filter((p) => p.startsWith(":"))
.map((p) => p.replace(/^:/, "").replace(/[+*]$/, "")),
patternParts: parts,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ultra-minor: this mockRoute helper manually constructs every field of AppRoute. If AppRoute gains new required fields in the future, this will silently fail to compile or — worse — produce a confusing test failure rather than a clear type error.

Consider adding satisfies AppRoute to the return (TypeScript 4.9+) so the compiler catches any shape drift immediately:

Suggested change
patternParts: parts,
} satisfies AppRoute;

@ask-bonk
Copy link
Copy Markdown
Contributor

ask-bonk bot commented Mar 30, 2026

Review submitted. Here's a summary of my review:

Approved the PR. The refactor is clean and correct:

  • O(1) Map lookup replacing allRoutes.find() is a sound improvement
  • Incremental prefix string building is equivalent to the old slice/join and avoids per-iteration allocations
  • Pre-computed lastDynamicIdx via reverse scan replaces the per-iteration slice().some() allocation
  • Dead ParentSegment.params field removal eliminates an allocated-but-never-read field
  • StaticParamsMap type export reduces duplication between the call site and the function signature

The 9 unit tests cover the key edge cases well (no parents, missing pagePath, missing generateStaticParams, single parent, two-level nesting, static segments between dynamic parents, fully static routes, single-segment dynamic, catch-all child).

I left one additional minor suggestion: adding satisfies AppRoute to the mockRoute return in tests so the compiler catches any future AppRoute shape drift. The two nits from the previous review (detached JSDoc, part reuse line swap) are also worth addressing but not blocking.

github run

@james-elicx james-elicx merged commit 33dfe26 into cloudflare:main Mar 30, 2026
24 checks passed
@NathanDrake2406 NathanDrake2406 deleted the perf/resolve-parent-params branch March 30, 2026 23:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants