WIP: fix(router-devtools-core): ship SSR-safe build to avoid window crash#7207
Open
brenelz wants to merge 1 commit intoTanStack:mainfrom
Open
WIP: fix(router-devtools-core): ship SSR-safe build to avoid window crash#7207brenelz wants to merge 1 commit intoTanStack:mainfrom
brenelz wants to merge 1 commit intoTanStack:mainfrom
Conversation
vite-plugin-solid's DOM JSX transform emits module-scope delegateEvents()
and template() calls that read window.document at import time, crashing
SSR consumers (nitro, Node) as soon as the pre-bundled chunks are loaded.
Produce a second build with solid({ ssr: true }) + build.ssr, emitted to
dist/esm/index.server.js, and expose it via the 'node' export condition
so server bundlers pick it up automatically.
Closes TanStack#7205
Contributor
|
View your CI Pipeline Execution ↗ for commit 18809d9
☁️ Nx Cloud last updated this comment at |
Contributor
📝 WalkthroughWalkthroughThe package now supports SSR-safe builds by introducing a separate server entry point and environment-specific export conditions. The build process generates both client and server outputs, with package.json exports directing consumers to the appropriate bundle based on their environment. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Contributor
Bundle Size Benchmarks
Trend sparkline is historical gzip bytes ending with this PR measurement; lower is better. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/router-devtools-core/package.json (1)
44-55: Consider a server-safe fallback for non-Node SSR runtimes.The
defaultcondition falls back to the client build, so SSR runtimes that don't assert thenodecondition (e.g., Deno by default, some edge/SSR pipelines) will still loadindex.jsand hit the originalwindowcrash. If Deno/edge SSR support matters for devtools, either add adeno/workerdmapping toindex.server.js, or swap the fallback so unknown environments prefer the safer server bundle.💡 Example (explicit Deno mapping)
"import": { "types": "./dist/esm/index.d.ts", "worker": "./dist/esm/index.js", "browser": "./dist/esm/index.js", + "deno": "./dist/esm/index.server.js", "node": "./dist/esm/index.server.js", "default": "./dist/esm/index.js" },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/router-devtools-core/package.json` around lines 44 - 55, The package.json exports currently fall back to the client bundle via the "default" condition which causes non-Node SSR runtimes to load index.js and crash; update the exports so unknown environments prefer the server-safe bundle (index.server.js) or add explicit SSR runtime conditions (e.g., "deno" or "workerd") that map to ./dist/esm/index.server.js; modify the "." export object (keys "default", "node", "browser", "import") to either change "default" to ./dist/esm/index.server.js or add new "deno"/"workerd" conditions pointing to ./dist/esm/index.server.js while keeping "browser" -> index.js for client builds.packages/router-devtools-core/vite.config.ts (1)
60-62: Avoidanyon the plugin predicate.Typing
pasanydisables the strict type-safety the repo targets. Vite'sPlugin/PluginOptiontype already covers falsy entries and has anamefield, so an explicit import keeps this safe without losing readability.🧹 Suggested change
-import solid from 'vite-plugin-solid' +import solid from 'vite-plugin-solid' +import type { PluginOption } from 'vite' @@ - merged.plugins = merged.plugins.filter( - (p: any) => !p || p.name !== 'vite:dts', - ) + merged.plugins = (merged.plugins as Array<PluginOption>).filter( + (p) => !p || typeof p !== 'object' || !('name' in p) || p.name !== 'vite:dts', + )As per coding guidelines: "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-devtools-core/vite.config.ts` around lines 60 - 62, The filter currently types the parameter as any; change it to use Vite's Plugin (or PluginOption) types instead and add a proper nullable type guard so the predicate is type-safe; import { Plugin } from 'vite' and update the predicate on merged.plugins.filter to accept p as Plugin | null | undefined (or use a type guard like p is Plugin) and check p.name !== 'vite:dts' while accounting for falsy p, targeting the existing merged.plugins filter expression and the 'vite:dts' plugin name.
🤖 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-devtools-core/vite.config.ts`:
- Around line 43-57: The SSR build is writing un-hashed chunks into the same
dist/esm/ folder and can overwrite client chunks; update the Vite SSR output
config (merged.build.ssr and merged.build.rolldownOptions.output) so SSR
artifacts are written to a distinct namespace or include a hash: change
merged.build.rolldownOptions.output.chunkFileNames and entryFileNames to use
either an SSR subdirectory (e.g., 'esm-ssr/[name].js') or include a content/hash
token (e.g., 'esm/[name].[hash].js') so SSR chunks cannot collide with client
chunks; ensure both merged.build.rolldownOptions.output.entryFileNames and
chunkFileNames are updated consistently.
---
Nitpick comments:
In `@packages/router-devtools-core/package.json`:
- Around line 44-55: The package.json exports currently fall back to the client
bundle via the "default" condition which causes non-Node SSR runtimes to load
index.js and crash; update the exports so unknown environments prefer the
server-safe bundle (index.server.js) or add explicit SSR runtime conditions
(e.g., "deno" or "workerd") that map to ./dist/esm/index.server.js; modify the
"." export object (keys "default", "node", "browser", "import") to either change
"default" to ./dist/esm/index.server.js or add new "deno"/"workerd" conditions
pointing to ./dist/esm/index.server.js while keeping "browser" -> index.js for
client builds.
In `@packages/router-devtools-core/vite.config.ts`:
- Around line 60-62: The filter currently types the parameter as any; change it
to use Vite's Plugin (or PluginOption) types instead and add a proper nullable
type guard so the predicate is type-safe; import { Plugin } from 'vite' and
update the predicate on merged.plugins.filter to accept p as Plugin | null |
undefined (or use a type guard like p is Plugin) and check p.name !== 'vite:dts'
while accounting for falsy p, targeting the existing merged.plugins filter
expression and the 'vite:dts' plugin name.
🪄 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: 40b19a03-ad75-4154-b038-b4c216e02563
📒 Files selected for processing (2)
packages/router-devtools-core/package.jsonpackages/router-devtools-core/vite.config.ts
Comment on lines
+43
to
+57
| if (ssr) { | ||
| // Don't wipe the client build produced in the previous step. | ||
| merged.build.emptyOutDir = false | ||
| merged.build.lib.formats = ['es'] | ||
| // Build in SSR mode so Vite resolves `solid-js/web` via the `node` export | ||
| // condition and pulls in Solid's server build. | ||
| merged.build.ssr = true | ||
| merged.ssr = { | ||
| // Still bundle Solid so the server artifact is self-contained. | ||
| noExternal: ['solid-js', 'solid-js/web'], | ||
| } | ||
| // `lib.fileName` isn't honored when `build.ssr` is set, so drive the | ||
| // output names via rolldown directly. | ||
| merged.build.rolldownOptions.output.entryFileNames = 'esm/index.server.js' | ||
| merged.build.rolldownOptions.output.chunkFileNames = 'esm/[name].js' |
Contributor
There was a problem hiding this comment.
SSR chunks can overwrite client chunks in dist/esm/.
With emptyOutDir = false the SSR step writes into the same dist/esm/ directory as the client build, and chunkFileNames = 'esm/[name].js' has no hash and no SSR-specific namespace. Today it happens to produce a single bundle (single entry, noExternal for Solid, no manualChunks), but any future dynamic-import or code-split would silently overwrite same-named client chunks with SSR-compiled content — causing "window is not defined" at runtime on the browser side.
Prefer either a hashed name or an SSR-scoped subdirectory for chunks.
🛡️ Defensive fix
merged.build.rolldownOptions.output.entryFileNames = 'esm/index.server.js'
- merged.build.rolldownOptions.output.chunkFileNames = 'esm/[name].js'
+ merged.build.rolldownOptions.output.chunkFileNames = 'esm/server/[name]-[hash].js'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (ssr) { | |
| // Don't wipe the client build produced in the previous step. | |
| merged.build.emptyOutDir = false | |
| merged.build.lib.formats = ['es'] | |
| // Build in SSR mode so Vite resolves `solid-js/web` via the `node` export | |
| // condition and pulls in Solid's server build. | |
| merged.build.ssr = true | |
| merged.ssr = { | |
| // Still bundle Solid so the server artifact is self-contained. | |
| noExternal: ['solid-js', 'solid-js/web'], | |
| } | |
| // `lib.fileName` isn't honored when `build.ssr` is set, so drive the | |
| // output names via rolldown directly. | |
| merged.build.rolldownOptions.output.entryFileNames = 'esm/index.server.js' | |
| merged.build.rolldownOptions.output.chunkFileNames = 'esm/[name].js' | |
| if (ssr) { | |
| // Don't wipe the client build produced in the previous step. | |
| merged.build.emptyOutDir = false | |
| merged.build.lib.formats = ['es'] | |
| // Build in SSR mode so Vite resolves `solid-js/web` via the `node` export | |
| // condition and pulls in Solid's server build. | |
| merged.build.ssr = true | |
| merged.ssr = { | |
| // Still bundle Solid so the server artifact is self-contained. | |
| noExternal: ['solid-js', 'solid-js/web'], | |
| } | |
| // `lib.fileName` isn't honored when `build.ssr` is set, so drive the | |
| // output names via rolldown directly. | |
| merged.build.rolldownOptions.output.entryFileNames = 'esm/index.server.js' | |
| merged.build.rolldownOptions.output.chunkFileNames = 'esm/server/[name]-[hash].js' |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/router-devtools-core/vite.config.ts` around lines 43 - 57, The SSR
build is writing un-hashed chunks into the same dist/esm/ folder and can
overwrite client chunks; update the Vite SSR output config (merged.build.ssr and
merged.build.rolldownOptions.output) so SSR artifacts are written to a distinct
namespace or include a hash: change
merged.build.rolldownOptions.output.chunkFileNames and entryFileNames to use
either an SSR subdirectory (e.g., 'esm-ssr/[name].js') or include a content/hash
token (e.g., 'esm/[name].[hash].js') so SSR chunks cannot collide with client
chunks; ensure both merged.build.rolldownOptions.output.entryFileNames and
chunkFileNames are updated consistently.
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
1 participant
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.
vite-plugin-solid's DOM JSX transform emits module-scope delegateEvents() and template() calls that read window.document at import time, crashing SSR consumers (nitro, Node) as soon as the pre-bundled chunks are loaded.
Produce a second build with solid({ ssr: true }) + build.ssr, emitted to dist/esm/index.server.js, and expose it via the 'node' export condition so server bundlers pick it up automatically.
Closes #7205
Summary by CodeRabbit