-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat(nextjs): Add routeManifestInjection option to exclude routes from client bundle #18798
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(nextjs): Add routeManifestInjection option to exclude routes from client bundle #18798
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR introduces a new routeManifestInjection configuration option that provides fine-grained control over which routes are included in the route manifest injected into the client bundle, addressing security concerns about exposing sensitive route patterns. The PR also deprecates the existing disableManifestInjection option in favor of the more flexible new option.
Changes:
- Added
routeManifestInjectionoption supportingfalse, object withexcludearray/function, providing exact string matching, regex patterns, and predicate function filtering - Deprecated
disableManifestInjectionoption with a console warning - Implemented route filtering logic in
maybeCreateRouteManifestto apply exclusion rules to static routes, dynamic routes, and ISR routes
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/nextjs/src/config/types.ts | Added new routeManifestInjection type definition with documentation and examples, deprecated disableManifestInjection |
| packages/nextjs/src/config/withSentryConfig/getFinalConfigObjectUtils.ts | Implemented deprecation warning, option precedence handling, and route filtering logic based on exclude patterns |
| packages/nextjs/test/config/manifest/excludeRoutesFromManifest.test.ts | Added comprehensive unit tests for route exclusion filtering with string, regex, function, and edge case scenarios |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
packages/nextjs/src/config/withSentryConfig/getFinalConfigObjectUtils.ts
Show resolved
Hide resolved
packages/nextjs/src/config/withSentryConfig/getFinalConfigObjectUtils.ts
Show resolved
Hide resolved
packages/nextjs/test/config/manifest/excludeRoutesFromManifest.test.ts
Outdated
Show resolved
Hide resolved
packages/nextjs/src/config/withSentryConfig/getFinalConfigObjectUtils.ts
Outdated
Show resolved
Hide resolved
packages/nextjs/src/config/withSentryConfig/getFinalConfigObjectUtils.ts
Outdated
Show resolved
Hide resolved
packages/nextjs/src/config/withSentryConfig/getFinalConfigObjectUtils.ts
Outdated
Show resolved
Hide resolved
node-overhead report 🧳Note: This is a synthetic benchmark with a minimal express app and does not necessarily reflect the real-world performance impact in an application.
|
chargome
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looks good, just left a comment but feel free to ignore.
Thanks for shipping this!
|
|
||
| return excludeFilter.some(pattern => { | ||
| if (typeof pattern === 'string') { | ||
| return route === pattern; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want exact matching here? Easy to mess up with trailing slashes etc – maybe an includes would be more suitable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I initially thought that strings should be precise, but then found a isMatchingPattern utility that uses includes for this case.
So for consistency I will use that util, and users can do precise matching with fully qualified URLs or Regexs anyways. Thanks for calling it out!
| /** | ||
| * Disables automatic injection of the route manifest into the client bundle. | ||
| * | ||
| * @deprecated Use `routeManifestInjection: false` instead. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a todo(v11) comment for removal pls. We should add a tracking issue for v11 for removing all the deprecated types then
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, I will also add it for the other deprecated stuff.
|
Merging since remix hydro tests are flakey atm |
…t for specific pages
8db1794 to
fba9871
Compare
Added a new
routeManifestInjectionconfiguration option that allows users to exclude specific routes from the route manifest injected into the client bundle. This addresses concerns about sensitive or unreleased route patterns being exposed in the client-side code.This also deprecated
disableManifestInjectionoption since it would be possible to have conflicting options present which wouldn't be a great DX.Users can disable it entirely by passing
false, otherwise they can use an object with anexcludeproperty. The property can be an array of string/regex values, or a predicate function.The value typings prevent disabling the manifest and excluding it at the same time, also deprecation annotations and build-time warnings should point users towards the new option.
closes #18713