Skip to content

Conversation

@danielroe
Copy link
Member

πŸ”— Linked issue

❓ Type of change

  • 🐞 Bug fix (a non-breaking change that fixes an issue)

πŸ“š Description

noticed that if we call nitro.routing.routeRules.compileToString with different options from the ones nitro passes to it, the wrong output can be cached.

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

@danielroe danielroe requested a review from pi0 as a code owner December 19, 2025 10:06
@vercel
Copy link

vercel bot commented Dec 19, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Review Updated (UTC)
nitro.build Ready Ready Preview, Comment Dec 19, 2025 10:06am

@coderabbitai
Copy link

coderabbitai bot commented Dec 19, 2025

πŸ“ Walkthrough

Walkthrough

The _compiled property in the Router class is refactored from storing a single cached compiled string to storing a map of strings keyed by options. The compileToString method now implements per-options caching, generating option keys and retrieving/storing compiled results per key.

Changes

Cohort / File(s) Summary
Router caching refactor
src/routing.ts
Changed _compiled property from string to Record<string, string> to support per-options caching. Updated compileToString method to compute option keys, initialize the map, check cache by key, and store/retrieve compiled strings per key instead of maintaining a single cached value.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Per-options key generation: Verify that the key computation from options is deterministic and correctly differentiates distinct option combinations
  • Cache initialization and lookup logic: Confirm the map is properly initialized on first call and subsequent cache hits/misses work as intended
  • Wildcard route optimization: Ensure the per-key cache storage for single wildcard routes maintains correct behavior and doesn't introduce edge cases
  • Type compatibility: Review all call sites to ensure they correctly handle the new Record<string, string> type for _compiled

Pre-merge checks and finishing touches

βœ… Passed checks (3 passed)
Check name Status Explanation
Title check βœ… Passed The title follows conventional commits format with 'fix:' prefix and clearly describes the main change (caching by options hash).
Description check βœ… Passed The description is related to the changeset, explaining the bug being fixed (caching issue with different options).
Docstring Coverage βœ… Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
  • πŸ“ Generate docstrings
πŸ§ͺ Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/cached-compile

πŸ“œ Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

πŸ“₯ Commits

Reviewing files that changed from the base of the PR and between 3992c73 and 68deb13.

πŸ“’ Files selected for processing (1)
  • src/routing.ts (3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: tests-rollup (ubuntu-latest)
  • GitHub Check: tests-rollup (windows-latest)
  • GitHub Check: tests-rolldown (windows-latest)
πŸ”‡ Additional comments (3)
src/routing.ts (3)

147-147: LGTM! Type change enables per-options caching.

The change from string to Record<string, string> correctly supports storing multiple cached compilations keyed by options.


203-204: LGTM! Wildcard optimization correctly updated.

The single wildcard route optimization now stores its result in the per-key cache, consistent with the new caching strategy.


184-189: Cache implementation is sound. The per-options caching logic is correctly implemented:

  • Deterministic key generation using hash(opts) from ohash, which handles the RouterCompilerOptions type correctly
  • Proper lazy initialization and cache lookup with ||= pattern
  • Cache invalidation on route updates (sets _compiled to undefined, which triggers reinitialization on next call)
  • No external code accesses the private _compiled property

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.

❀️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@pi0 pi0 merged commit 00598a8 into main Dec 19, 2025
11 checks passed
@pi0 pi0 deleted the fix/cached-compile branch December 19, 2025 12:36
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.

3 participants