fix(router-plugin): isolate route metadata per plugin instance#7313
fix(router-plugin): isolate route metadata per plugin instance#7313schiller-manuel merged 3 commits intomainfrom
Conversation
📝 WalkthroughWalkthroughRouter plugin state is moved from a shared global ( Changes
Sequence Diagram(s)sequenceDiagram
participant User as Vite Config / Consumer
participant Composed as Composed Factory
participant Context as RouterPluginContext
participant Generator as Router Generator Plugin
participant Splitter as Code-Splitter Plugin
participant HMR as Router HMR Plugin
participant RouteMap as context.routesByFile
User->>Composed: call tanstackRouter(options)
Composed->>Context: createRouterPluginContext()
Composed->>Generator: createRouterGeneratorPlugin(options, Context)
Generator->>RouteMap: write routes (generator.getRoutesByFileMap())
Composed->>Splitter: createRouterCodeSplitterPlugin(options, Context)
Composed->>HMR: createRouterHmrPlugin(options, Context)
Splitter->>RouteMap: read routes for transform
HMR->>RouteMap: read routes for HMR handling
sequenceDiagram
participant InstanceA as Router Instance A
participant CtxA as Context A
participant InstanceB as Router Instance B
participant CtxB as Context B
participant File as Route File
InstanceA->>CtxA: createRouterPluginContext()
InstanceB->>CtxB: createRouterPluginContext()
InstanceA->>CtxA: generator writes routes for File
InstanceB->>CtxB: generator writes routes for other File
Note over CtxA,CtxB: Isolated maps prevent cross-transforms
File->>CtxA: Splitter A looks up File (only in CtxA)
File->>CtxB: Splitter B looks up File (only in CtxB)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Review rate limit: 4/5 reviews remaining, refill in 12 minutes. Comment |
|
View your CI Pipeline Execution ↗ for commit 6dfa56a
☁️ Nx Cloud last updated this comment at |
🚀 Changeset Version Preview2 package(s) bumped directly, 5 bumped as dependents. 🟩 Patch bumps
|
Bundle Size Benchmarks
Trend sparkline is historical gzip bytes ending with this PR measurement; lower is better. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/router-plugin/tests/router-plugin-context.test.ts (1)
78-146: ⚡ Quick winCover the public
tanstackRouter()wiring too.This regression came from the composed/plugin-entry path, but the test only exercises
createRouterCodeSplitterPlugindirectly. A small integration test that instantiatestanstackRouter()twice would guard the actual Vite API surface changed in this PR and catch future context-wiring regressions higher up the stack.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/router-plugin/tests/router-plugin-context.test.ts` around lines 78 - 146, Add an integration test that exercises the public tanstackRouter() wiring by instantiating tanstackRouter() twice (instead of calling createRouterCodeSplitterPlugin directly) and verifying their code-splitter contexts remain isolated; specifically, create two router instances via tanstackRouter(), obtain their splitter plugins (or plugin entries) which internally call createRouterCodeSplitterPlugin/createRouterPluginContext, run configurePlugin on both, then run transformReferenceRoute for the first instance and assert it produces one hot-declaration and that running transformReferenceRoute on the second instance with the same input returns null (use existing helpers like configurePlugin, transformReferenceRoute, getReferencePlugin, countProgramHotDeclarations to mirror the current test flow).
🤖 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-plugin/src/webpack.ts`:
- Around line 42-43: The plugin is using the module-level singleton
defaultRouterPluginContext as a parameter default (routerPluginContext:
RouterPluginContext = defaultRouterPluginContext), which causes routesByFile and
other metadata to be shared across plugin instances; change the API so the
default is undefined or create a per-instance clone inside the factory (e.g., in
the function that builds the plugin) and initialize routerPluginContext =
cloneDefaultRouterPluginContext() or create a fresh object with an isolated
routesByFile map when routerPluginContext is not provided; update all places
that used the parameter default (including the other occurrence at the parameter
around line 60) to avoid referencing the module-wide defaultRouterPluginContext
directly so each plugin instance gets its own context instance.
---
Nitpick comments:
In `@packages/router-plugin/tests/router-plugin-context.test.ts`:
- Around line 78-146: Add an integration test that exercises the public
tanstackRouter() wiring by instantiating tanstackRouter() twice (instead of
calling createRouterCodeSplitterPlugin directly) and verifying their
code-splitter contexts remain isolated; specifically, create two router
instances via tanstackRouter(), obtain their splitter plugins (or plugin
entries) which internally call
createRouterCodeSplitterPlugin/createRouterPluginContext, run configurePlugin on
both, then run transformReferenceRoute for the first instance and assert it
produces one hot-declaration and that running transformReferenceRoute on the
second instance with the same input returns null (use existing helpers like
configurePlugin, transformReferenceRoute, getReferencePlugin,
countProgramHotDeclarations to mirror the current test flow).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4b4d8c30-9d80-407e-b67d-ebac4f48ca07
📒 Files selected for processing (18)
.changeset/clean-router-plugin-context.mdpackages/router-plugin/package.jsonpackages/router-plugin/src/context.tspackages/router-plugin/src/core/router-code-splitter-plugin.tspackages/router-plugin/src/core/router-composed-plugin.tspackages/router-plugin/src/core/router-generator-plugin.tspackages/router-plugin/src/core/router-hmr-plugin.tspackages/router-plugin/src/core/router-plugin-context.tspackages/router-plugin/src/esbuild.tspackages/router-plugin/src/global.d.tspackages/router-plugin/src/index.tspackages/router-plugin/src/rspack.tspackages/router-plugin/src/vite.tspackages/router-plugin/src/webpack.tspackages/router-plugin/tests/router-plugin-context.test.tspackages/router-plugin/vite.config.tspackages/start-plugin-core/src/rsbuild/start-router-plugin.tspackages/start-plugin-core/src/vite/start-router-plugin/plugin.ts
💤 Files with no reviewable changes (1)
- packages/router-plugin/src/global.d.ts
Merging this PR will not alter performance
Comparing Footnotes
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/router-plugin/tests/router-plugin-context.test.ts (1)
79-180: ⚡ Quick winAdd one wrapper-level regression case.
This covers the core plugins well, but it never exercises the new public wrapper API. A small test that wires a shared
RouterPluginContextthroughTanStackRouterGenerator*andTanStackRouterCodeSplitter*would catch entrypoint-level regressions in the exported surface.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/router-plugin/tests/router-plugin-context.test.ts` around lines 79 - 180, Add a wrapper-level regression test that wires a single shared RouterPluginContext through the public wrapper exports (the TanStackRouterGenerator* and TanStackRouterCodeSplitter* entrypoints) instead of calling the internal plugin factories directly; create a RouterPluginContext via createRouterPluginContext(), populate routesByFile for a sample route file, instantiate the public wrapper generator and code-splitter (the exported TanStackRouterGenerator* and TanStackRouterCodeSplitter* functions/classes), run their configure/transform flows (similar to existing tests using createRouterCodeSplitterPlugin/createRouterHmrPlugin) and assert the generated output contains the expected HMR/fast-refresh anchor or code-splitting markers; this ensures you reference the public wrapper symbols rather than internal plugin constructors while keeping the test structure similar to the existing 'router plugin context' cases.
🤖 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-plugin/src/esbuild.ts`:
- Around line 23-30: The problem is that TanStackRouterGeneratorEsbuild and the
corresponding code-splitter create separate router plugin contexts (and thus
separate routesByFile maps) when callers omit routerPluginContext; fix by
creating a shared module-level default context and using it when
routerPluginContext is not provided. Concretely, add a top-level const (e.g.,
defaultRouterPluginContext = createRouterPluginContext()) and change the
fallback in TanStackRouterGeneratorEsbuild (and
TanStackRouterCodeSplitterEsbuild) from routerPluginContext ??
createRouterPluginContext() to routerPluginContext ?? defaultRouterPluginContext
so both plugins share the same routesByFile map populated by
createRouterPluginContext.
In `@packages/router-plugin/src/rspack.ts`:
- Around line 55-66: The two exports TanStackRouterGeneratorRspack and
TanStackRouterCodeSplitterRspack each create a fresh RouterPluginContext which
breaks metadata sharing; fix by introducing a single module-level default
context (e.g., a const defaultRouterPluginContext = createRouterPluginContext())
and change both functions to use routerPluginContext ??
defaultRouterPluginContext so callers can still pass a context but the default
is shared across both generator and code-splitter; update both function bodies
(references to pluginContext in TanStackRouterGeneratorRspack and
TanStackRouterCodeSplitterRspack) to use this shared default.
In `@packages/router-plugin/src/webpack.ts`:
- Around line 40-48: Both TanStackRouterGeneratorWebpack and
TanStackRouterCodeSplitterWebpack create fresh RouterPluginContext instances
when none is passed, causing route metadata to be isolated between the two
exporters; fix by creating a single module-scoped default context (e.g.,
defaultRouterPluginContext = createRouterPluginContext()) and have both
TanStackRouterGeneratorWebpack and TanStackRouterCodeSplitterWebpack use that
default when routerPluginContext is undefined (instead of calling
createRouterPluginContext() inside each function), ensuring
createRouterGeneratorPlugin and the code-splitter share the same context unless
a caller explicitly provides one.
---
Nitpick comments:
In `@packages/router-plugin/tests/router-plugin-context.test.ts`:
- Around line 79-180: Add a wrapper-level regression test that wires a single
shared RouterPluginContext through the public wrapper exports (the
TanStackRouterGenerator* and TanStackRouterCodeSplitter* entrypoints) instead of
calling the internal plugin factories directly; create a RouterPluginContext via
createRouterPluginContext(), populate routesByFile for a sample route file,
instantiate the public wrapper generator and code-splitter (the exported
TanStackRouterGenerator* and TanStackRouterCodeSplitter* functions/classes), run
their configure/transform flows (similar to existing tests using
createRouterCodeSplitterPlugin/createRouterHmrPlugin) and assert the generated
output contains the expected HMR/fast-refresh anchor or code-splitting markers;
this ensures you reference the public wrapper symbols rather than internal
plugin constructors while keeping the test structure similar to the existing
'router plugin context' cases.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7fbe4b23-1a2f-4b17-8aec-132a647c4de8
📒 Files selected for processing (9)
packages/router-plugin/src/core/router-code-splitter-plugin.tspackages/router-plugin/src/core/router-generator-plugin.tspackages/router-plugin/src/core/router-hmr-plugin.tspackages/router-plugin/src/core/router-plugin-context.tspackages/router-plugin/src/esbuild.tspackages/router-plugin/src/rspack.tspackages/router-plugin/src/vite.tspackages/router-plugin/src/webpack.tspackages/router-plugin/tests/router-plugin-context.test.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/router-plugin/src/core/router-plugin-context.ts
- packages/router-plugin/src/core/router-hmr-plugin.ts
- packages/router-plugin/src/core/router-generator-plugin.ts
- packages/router-plugin/src/core/router-code-splitter-plugin.ts
- packages/router-plugin/src/vite.ts
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
packages/router-plugin/src/vite.ts (1)
11-11: ⚡ Quick winThe new options alias drops strict config typing.
Partial<Config | (() => Config)>is not the same as “Partial<Config>or a config factory”. It weakens the public signature and makes it much easier for invalid option shapes to slip through unchecked. Keep this as an explicit union instead of wrapping the whole union inPartial.Suggested type fix
-type RouterPluginOptions = Partial<Config | (() => Config)> | undefined +type RouterPluginOptions = Partial<Config> | (() => Partial<Config>) | undefinedAs per coding guidelines,
**/*.{ts,tsx}: Use TypeScript strict mode with extensive type safety.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/router-plugin/src/vite.ts` at line 11, The RouterPluginOptions alias currently wraps the union in Partial which weakens typing; change the type so it is an explicit union of a partial config OR a config factory (e.g., replace the current RouterPluginOptions definition with an explicit union like Partial<Config> | (() => Config) | undefined) so that Config itself remains strictly typed; update the type declaration for RouterPluginOptions in packages/router-plugin/src/vite.ts accordingly and ensure references to RouterPluginOptions and Config still compile under strict TypeScript.packages/router-plugin/src/esbuild.ts (1)
12-12: ⚡ Quick win
RouterPluginOptionsis typed too loosely for a public API.
Partial<Config | (() => Config)>does not model “partial config or config factory” correctly; it drops the intended shape and weakens consumer-side checking. This should stay a real union such asPartial<Config> | (() => Partial<Config>) | undefinedor whatever exact input shape the core helpers accept.Suggested type fix
-type RouterPluginOptions = Partial<Config | (() => Config)> | undefined +type RouterPluginOptions = Partial<Config> | (() => Partial<Config>) | undefinedAs per coding guidelines,
**/*.{ts,tsx}: Use TypeScript strict mode with extensive type safety.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/router-plugin/src/esbuild.ts` at line 12, RouterPluginOptions is currently too loose as Partial<Config | (() => Config)>; change it to an explicit union that preserves either a partial config or a config factory, e.g. declare RouterPluginOptions as Partial<Config> | (() => Partial<Config>) | undefined (or the exact factory return type your core helpers expect) so consumers get proper type-checking; update the type alias RouterPluginOptions in esbuild.ts and ensure it matches the shape accepted by the core helpers that consume Config.
🤖 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-plugin/src/esbuild.ts`:
- Around line 14-15: The module-level defaultRouterPluginContext (created by
createRouterPluginContext) causes all
TanStackRouterGeneratorEsbuild/TanStackRouterCodeSplitterEsbuild instances to
share the same routesByFile namespace; change to create a fresh context per
instance (or scope routesByFile by resolved routesDirectory) by moving the
createRouterPluginContext call into the constructors/factory functions of
TanStackRouterGeneratorEsbuild and TanStackRouterCodeSplitterEsbuild (or by
keying the shared map by routesDirectory before any transform decision) so each
pair gets an isolated context and cannot see or collide with another pair’s
metadata.
In `@packages/router-plugin/src/rspack.ts`:
- Around line 13-14: The module-level defaultRouterPluginContext creates a
process-wide routesByFile map that causes standalone
TanStackRouterGeneratorRspack/TanStackRouterCodeSplitterRspack calls to share
metadata; instead, remove the shared module-level default and instantiate
createRouterPluginContext() per plugin instance when a context isn't provided.
Update the factory functions TanStackRouterGeneratorRspack and
TanStackRouterCodeSplitterRspack to call createRouterPluginContext() inside the
function (or accept and clone a provided context) so each invocation gets its
own context/ routesByFile map; replace usages of defaultRouterPluginContext
across the file (including the other occurrences you noted) to use the
instance-local context creation. Ensure semantics remain the same when a
user-supplied context is passed.
In `@packages/router-plugin/src/vite.ts`:
- Line 13: The module-level defaultRouterPluginContext created by
createRouterPluginContext() causes shared metadata across independent Vite
pairs; change to create per-pair context by removing the single module-level
instance and instead lazily create or cache a RouterPluginContext keyed by the
resolved routes directory (or create a fresh context each time) when
tanstackRouterGenerator() or tanStackRouterCodeSplitter() are invoked without an
explicit context; update those functions to call
createRouterPluginContext(resolvedRoutesDir) or use an internal Map keyed by
resolved routes dir to isolate metadata namespaces so each Vite pair gets its
own RouterPluginContext.
---
Nitpick comments:
In `@packages/router-plugin/src/esbuild.ts`:
- Line 12: RouterPluginOptions is currently too loose as Partial<Config | (() =>
Config)>; change it to an explicit union that preserves either a partial config
or a config factory, e.g. declare RouterPluginOptions as Partial<Config> | (()
=> Partial<Config>) | undefined (or the exact factory return type your core
helpers expect) so consumers get proper type-checking; update the type alias
RouterPluginOptions in esbuild.ts and ensure it matches the shape accepted by
the core helpers that consume Config.
In `@packages/router-plugin/src/vite.ts`:
- Line 11: The RouterPluginOptions alias currently wraps the union in Partial
which weakens typing; change the type so it is an explicit union of a partial
config OR a config factory (e.g., replace the current RouterPluginOptions
definition with an explicit union like Partial<Config> | (() => Config) |
undefined) so that Config itself remains strictly typed; update the type
declaration for RouterPluginOptions in packages/router-plugin/src/vite.ts
accordingly and ensure references to RouterPluginOptions and Config still
compile under strict TypeScript.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f7ef2660-513e-4ed3-8d2f-1ad04380008b
📒 Files selected for processing (4)
packages/router-plugin/src/esbuild.tspackages/router-plugin/src/rspack.tspackages/router-plugin/src/vite.tspackages/router-plugin/src/webpack.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/router-plugin/src/webpack.ts
| const defaultRouterPluginContext = createRouterPluginContext() | ||
|
|
There was a problem hiding this comment.
Module-level fallback context still couples standalone instances.
Line 14 makes every default TanStackRouterGeneratorEsbuild() / TanStackRouterCodeSplitterEsbuild() call share the same routesByFile namespace. If a config registers two standalone generator/code-splitter pairs, both splitters can still see the other pair’s metadata and hit the same double-transform path this PR is fixing. Use a fresh default context per pair/instance, or namespace the shared map by resolved routesDirectory before transform decisions are made.
Also applies to: 25-32, 44-52
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/router-plugin/src/esbuild.ts` around lines 14 - 15, The module-level
defaultRouterPluginContext (created by createRouterPluginContext) causes all
TanStackRouterGeneratorEsbuild/TanStackRouterCodeSplitterEsbuild instances to
share the same routesByFile namespace; change to create a fresh context per
instance (or scope routesByFile by resolved routesDirectory) by moving the
createRouterPluginContext call into the constructors/factory functions of
TanStackRouterGeneratorEsbuild and TanStackRouterCodeSplitterEsbuild (or by
keying the shared map by routesDirectory before any transform decision) so each
pair gets an isolated context and cannot see or collide with another pair’s
metadata.
| const defaultRouterPluginContext = createRouterPluginContext() | ||
|
|
There was a problem hiding this comment.
Rspack standalone wrappers still share one metadata namespace by default.
The module-level defaultRouterPluginContext keeps every default TanStackRouterGeneratorRspack() / TanStackRouterCodeSplitterRspack() call on the same routesByFile map. That preserves pairing for one generator/splitter set, but two standalone pairs in the same config can still bleed into each other and trigger cross-instance transforms. This should be isolated per pair/instance instead of falling back to a single process-wide context.
Also applies to: 57-68, 83-96
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/router-plugin/src/rspack.ts` around lines 13 - 14, The module-level
defaultRouterPluginContext creates a process-wide routesByFile map that causes
standalone TanStackRouterGeneratorRspack/TanStackRouterCodeSplitterRspack calls
to share metadata; instead, remove the shared module-level default and
instantiate createRouterPluginContext() per plugin instance when a context isn't
provided. Update the factory functions TanStackRouterGeneratorRspack and
TanStackRouterCodeSplitterRspack to call createRouterPluginContext() inside the
function (or accept and clone a provided context) so each invocation gets its
own context/ routesByFile map; replace usages of defaultRouterPluginContext
across the file (including the other occurrences you noted) to use the
instance-local context creation. Ensure semantics remain the same when a
user-supplied context is passed.
|
|
||
| type RouterPluginOptions = Partial<Config | (() => Config)> | undefined | ||
|
|
||
| const defaultRouterPluginContext = createRouterPluginContext() |
There was a problem hiding this comment.
Shared default context defeats isolation for standalone Vite pairs.
Because Line 13 creates one module-level RouterPluginContext, every default tanstackRouterGenerator() / tanStackRouterCodeSplitter() instance ends up in the same metadata namespace. Two standalone pairs in one Vite config can still observe each other’s route records and reintroduce cross-instance transforms. This needs a per-pair default or an internal namespace keyed by the resolved routes directory.
Also applies to: 24-32, 43-51
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/router-plugin/src/vite.ts` at line 13, The module-level
defaultRouterPluginContext created by createRouterPluginContext() causes shared
metadata across independent Vite pairs; change to create per-pair context by
removing the single module-level instance and instead lazily create or cache a
RouterPluginContext keyed by the resolved routes directory (or create a fresh
context each time) when tanstackRouterGenerator() or
tanStackRouterCodeSplitter() are invoked without an explicit context; update
those functions to call createRouterPluginContext(resolvedRoutesDir) or use an
internal Map keyed by resolved routes dir to isolate metadata namespaces so each
Vite pair gets its own RouterPluginContext.
There was a problem hiding this comment.
Nx Cloud has identified a flaky task in your failed CI:
🔂 Since the failure was identified as flaky, we triggered a CI rerun by adding an empty commit to this branch.
🎓 Learn more about Self-Healing CI on nx.dev
fixes #7174
Summary by CodeRabbit
New Features
Bug Fixes
Tests