fix(init): scaffold ssr.noExternal for React Router v8 projects#374
fix(init): scaffold ssr.noExternal for React Router v8 projects#374manovotny wants to merge 1 commit into
Conversation
React Router 8 ships development/production conditional exports. `react-router dev` externalizes @clerk/react-router for SSR, so it resolves react-router's production build while app code gets the development build — two module instances, two Router contexts — and every SSR render throws "useNavigate() may be used only in the context of a <Router>" (remix-run/react-router#15232). A clerk init'd RR8 app was broken in dev out of the box. Scaffold ssr: { noExternal: ["@clerk/react-router"] } into the vite config for react-router >= 8 (v7 has no conditional exports and needs nothing). Emits a manual post-instruction when the config is missing or uses a function form magicast can't safely modify. Verified against a live repro: create-react-router@latest (RR 8.0.0) + @clerk/react-router@3.5.5 fails in dev SSR without the entry and renders cleanly with it; production build unaffected. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: e96261a The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Companion PRs, now all open:
|
📝 WalkthroughWalkthroughThis PR adds scaffolding logic to the React Router v8 CLI initializer that automatically updates a project's Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant ReactRouterScaffold as reactRouter.scaffold
participant ViteScaffolder as scaffoldViteConfig
participant FileSystem
User->>ReactRouterScaffold: run init for React Router v8
ReactRouterScaffold->>ViteScaffolder: scaffoldViteConfig(ctx)
ViteScaffolder->>FileSystem: locate vite.config.ts
alt config found and `@clerk/react-router` absent
ViteScaffolder->>ViteScaffolder: addClerkNoExternal (AST or regex fallback)
ViteScaffolder-->>ReactRouterScaffold: modify action
else config missing or function-form
ViteScaffolder-->>ReactRouterScaffold: needsManualViteWire=true
end
ReactRouterScaffold->>ReactRouterScaffold: assemble actions list
alt needsManualViteWire
ReactRouterScaffold->>User: postInstructions with manual noExternal note
end
Related PRs Suggested labels Suggested reviewers 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Warning Review ran into problems🔥 ProblemsLinked repositories: Your configuration references 7 linked repositories, but your current plan allows 1. Analyzed Repository analysis: Couldn't refresh Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/cli-core/src/commands/init/frameworks/react-router.test.ts (1)
461-483: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winStrengthen assertions to catch duplicate
ssrkeys, not just substring presence.This test only checks that the output contains
"some-other-pkg"and"@clerk/react-router"as substrings. It wouldn't catch a regression where the fix ends up in a duplicatessr:block that gets shadowed by the original one at runtime (see the related comment onaddClerkNoExternalinreact-router.ts). Consider asserting the actual parsed array (e.g., via a secondparseModuleonviteAction.contentand checkingssr.noExternalcontains exactly one@clerk/react-routeralongsidesome-other-pkg), or at least asserting there's only a singlessr:occurrence in the output.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli-core/src/commands/init/frameworks/react-router.test.ts` around lines 461 - 483, Strengthen the React Router 8 scaffold test so it verifies the final Vite config structure, not just raw substrings. In react-router.test.ts, update the assertion around reactRouter.scaffold and viteAction.content to parse the generated config (or otherwise inspect structure) and confirm ssr.noExternal contains both some-other-pkg and exactly one `@clerk/react-router` entry, with only a single ssr block present. Use the existing addClerkNoExternal / reactRouter.scaffold / viteAction checks to locate the test and make the assertion resistant to duplicate ssr keys.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/cli-core/src/commands/init/frameworks/react-router.ts`:
- Around line 434-444: The early skip in react-router init is using a naive
content.includes("`@clerk/react-router`") check that can falsely treat unrelated
mentions as already configured. Update the logic in the config inspection path
around the existing skip branch so it only skips after confirming
`@clerk/react-router` is actually present in the ssr.noExternal configuration
(preferably via the AST parse/inspection already used in this flow, or a much
narrower pattern tied to noExternal). If the match is only incidental, continue
with the normal wiring path and emit the manual Vite instruction instead of
returning skip from the current check.
- Around line 399-420: The addClerkNoExternal helper is mutating
mod.exports.default too early and the defineConfig fallback can create a
duplicate ssr block. Unwrap the defineConfig(...) argument first, then update
the existing ssr.noExternal array on that config object before generating code;
if ssr already exists, merge into it instead of injecting a second ssr key. Use
addClerkNoExternal, parseModule, and mod.exports.default to locate the fix.
---
Nitpick comments:
In `@packages/cli-core/src/commands/init/frameworks/react-router.test.ts`:
- Around line 461-483: Strengthen the React Router 8 scaffold test so it
verifies the final Vite config structure, not just raw substrings. In
react-router.test.ts, update the assertion around reactRouter.scaffold and
viteAction.content to parse the generated config (or otherwise inspect
structure) and confirm ssr.noExternal contains both some-other-pkg and exactly
one `@clerk/react-router` entry, with only a single ssr block present. Use the
existing addClerkNoExternal / reactRouter.scaffold / viteAction checks to locate
the test and make the assertion resistant to duplicate ssr keys.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5107ccbd-34ba-4b06-99d2-2a373394222c
📒 Files selected for processing (3)
.changeset/react-router-v8-noexternal.mdpackages/cli-core/src/commands/init/frameworks/react-router.test.tspackages/cli-core/src/commands/init/frameworks/react-router.ts
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
clerk/clerk_go(manual)
| function addClerkNoExternal(content: string): string { | ||
| try { | ||
| const mod = parseModule(content); | ||
| const defaultExport = mod.exports.default; | ||
| if (!defaultExport || typeof defaultExport !== "object") return content; | ||
|
|
||
| if (!defaultExport.ssr) defaultExport.ssr = {}; | ||
| if (!defaultExport.ssr.noExternal) defaultExport.ssr.noExternal = []; | ||
| if (!Array.isArray(defaultExport.ssr.noExternal)) return content; | ||
|
|
||
| defaultExport.ssr.noExternal.push("@clerk/react-router"); | ||
| return mod.generate().code; | ||
| } catch { | ||
| if (content.includes("defineConfig")) { | ||
| return content.replace( | ||
| /(defineConfig\s*\(\s*\{)/, | ||
| '$1\n ssr: {\n noExternal: ["@clerk/react-router"],\n },', | ||
| ); | ||
| } | ||
| return content; | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '== relevant files ==\n'
git ls-files | rg 'packages/cli-core/src/commands/init/frameworks/react-router\.ts|packages/cli-core.*test|packages/cli-core.*spec|package\.json|bun\.lock|pnpm-lock\.yaml|package-lock\.json|yarn.lock'
printf '\n== locate addClerkNoExternal ==\n'
rg -n "function addClerkNoExternal|addClerkNoExternal|defineConfig|noExternal" packages/cli-core/src/commands/init/frameworks/react-router.ts packages/cli-core -g '!**/node_modules/**'
printf '\n== package dependency for magicast ==\n'
rg -n '"magicast"|magicast' package.json packages/**/package.json bun.lock pnpm-lock.yaml package-lock.json yarn.lock -g '!**/node_modules/**'
printf '\n== tests mentioning react-router init ==\n'
rg -n "react-router|addClerkNoExternal|noExternal" packages/cli-core -g '*.{test,spec}.{ts,tsx,js,jsx}' -g '!**/node_modules/**' || trueRepository: clerk/cli
Length of output: 1931
🌐 Web query:
magicast defineConfig function-call export default defaultExport $type $args docs changelog
💡 Result:
Magicast is a tool for programmatically modifying JavaScript and TypeScript source code while preserving formatting [1][2]. When working with exports like defineConfig, Magicast provides mechanisms to inspect and manipulate function calls using special proxy-based properties [1][3]. Key concepts for working with function calls: $type: This property allows you to check if an export (or any node) is a function call [1][4]. For example, when interacting with export default defineConfig(...), you can verify the node type using mod.exports.default.$type === "function-call" [1]. $args: When a node is identified as a function call, its arguments are accessible via the .$args property [1][3]. This is an array where .$args[0] typically holds the configuration object passed to the function [1][4]. Usage Pattern: A common pattern when dealing with configuration files is to handle both raw object exports and function-wrapped exports (like defineConfig) by checking the type [1][4]: import { parseModule } from "magicast"; const mod = parseModule(export default defineConfig({ foo: 'bar' })); // Access the configuration object regardless of whether it is wrapped in a function call const options = mod.exports.default.$type === "function-call"? mod.exports.default.$args: mod.exports.default; console.log(options.foo); // "bar" Creating Function Calls: To create new function calls, you can use the builders utility provided by Magicast [1][5]. This allows you to programmatically define function calls and assign them to module exports [1][4]: import { builders } from "magicast"; // Create a function call 'create([1][6][7])' const call = builders.functionCall("create", [1][6][7]); mod.exports.default.list = call;
Citations:
- 1: https://github.com/unjs/magicast/blob/main/README.md
- 2: https://github.com/hairyf/skills/blob/HEAD/skills/unjs/references/features-magicast.md
- 3: refactor!: rename
.argumentsto.$argsfor functions unjs/magicast#7 - 4: https://context7.com/unjs/magicast/llms.txt
- 5: feat: construct function call unjs/magicast#15
- 6: https://www.npmjs.com/package/magicast
- 7: https://unjs.io/packages/magicast
🏁 Script executed (no clone):
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import sys, json, urllib.parse, urllib.request
q = urllib.parse.quote('magicast defineConfig function-call export default $type $args')
url = f'https://api.github.com/search/issues?q={q}+repo:unjs/magicast'
req = urllib.request.Request(url, headers={'Accept':'application/vnd.github+json','User-Agent':'Mozilla/5.0'})
try:
with urllib.request.urlopen(req, timeout=20) as r:
data = json.load(r)
print(json.dumps({
"total_count": data.get("total_count"),
"items": [
{"title": i["title"], "url": i["html_url"]}
for i in data.get("items", [])[:5]
]
}, indent=2))
except Exception as e:
print(type(e).__name__, e)
PY
Length of output: 146
🌐 Web query:
site:github.com/unjs/magicast README function-call $type $args defineConfig property assignment
💡 Result:
In the context of the Magicast library, $type and $args are internal properties used to represent and manipulate code structures, such as function calls like defineConfig, at an Abstract Syntax Tree (AST) level [1][2]. When Magicast parses code, it identifies certain structures as function calls. If a node is identified as a function call, its $type property is set to "function-call" [1]. The $args property is an array that contains the arguments passed to that function [1][2]. For example, when dealing with a configuration export like export default defineConfig({ foo: 'bar' }), you can programmatically interact with it by checking the $type and accessing the $args array [1]: import { parseModule } from "magicast"; const mod = parseModule(export default defineConfig({ foo: 'bar' })); // Check if the default export is a function call if (mod.exports.default.$type === "function-call") { // Access the first argument passed to the function const options = mod.exports.default.$args; console.log(options.foo); // 'bar' // Modify the property options.foo = 'baz'; } The use of the $ prefix (e.g., $type, $args) serves to distinguish these library-provided metadata properties from standard properties of the user's JavaScript objects, preventing naming collisions [2]. You can also use the builders utility to programmatically construct new function calls for modification or injection [1][3].
Citations:
- 1: https://github.com/unjs/magicast/blob/main/README.md
- 2: refactor!: rename
.argumentsto.$argsfor functions unjs/magicast#7 - 3: feat: construct function call unjs/magicast#15
🏁 Script executed (no clone):
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import json, urllib.parse, urllib.request
queries = [
"magicast function-call proxy property assignment defineConfig",
"unjs magicast $type function-call $args defineConfig",
]
for q in queries:
print("QUERY:", q)
url = "https://api.github.com/search/code?q=" + urllib.parse.quote(q + " repo:unjs/magicast")
req = urllib.request.Request(url, headers={'Accept':'application/vnd.github+json','User-Agent':'Mozilla/5.0'})
try:
with urllib.request.urlopen(req, timeout=20) as r:
data = json.load(r)
print(json.dumps({"total_count": data.get("total_count"), "items": [i.get("html_url") for i in data.get("items", [])[:5]]}, indent=2))
except Exception as e:
print(type(e).__name__, e)
PY
Length of output: 146
🏁 Script executed (no clone):
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import urllib.request, re
url = "https://raw.githubusercontent.com/unjs/magicast/main/README.md"
text = urllib.request.urlopen(url, timeout=20).read().decode()
for needle in ["defineConfig", "function-call", "$args", "exports.default"]:
print(f"\n== {needle} ==")
for m in re.finditer(needle, text, re.I):
start = max(0, m.start() - 220)
end = min(len(text), m.end() + 420)
print(text[start:end].replace("\n", " "))
print("---")
break
PY
Length of output: 146
Handle defineConfig(...) exports before mutating ssr.noExternal
mod.exports.default needs to unwrap the defineConfig argument before appending @clerk/react-router. As written, the fallback path can inject a second ssr key, and a later existing ssr block will win, dropping the new noExternal entry.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/cli-core/src/commands/init/frameworks/react-router.ts` around lines
399 - 420, The addClerkNoExternal helper is mutating mod.exports.default too
early and the defineConfig fallback can create a duplicate ssr block. Unwrap the
defineConfig(...) argument first, then update the existing ssr.noExternal array
on that config object before generating code; if ssr already exists, merge into
it instead of injecting a second ssr key. Use addClerkNoExternal, parseModule,
and mod.exports.default to locate the fix.
| const content = await Bun.file(join(ctx.cwd, configPath)).text(); | ||
| if (content.includes("@clerk/react-router")) { | ||
| return { | ||
| action: { | ||
| type: "skip", | ||
| path: configPath, | ||
| skipReason: "Already references @clerk/react-router in vite config", | ||
| }, | ||
| needsManualViteWire: false, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Naive substring check can cause a silent false-positive skip.
content.includes("@clerk/react-router") matches anywhere in the file (imports, comments, other config sections), not specifically inside ssr.noExternal. If it matches for an unrelated reason, the scaffold returns { action: skip, needsManualViteWire: false } — no automatic fix is applied and no manual post-instruction is emitted, so the SSR bug goes unresolved without any indication to the user.
Consider deferring the "already present" check to after attempting the AST parse (checking membership inside the actual ssr.noExternal array), or narrowing the regex to something like /noExternal\s*:\s*\[[^\]]*@clerk\/react-router/.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/cli-core/src/commands/init/frameworks/react-router.ts` around lines
434 - 444, The early skip in react-router init is using a naive
content.includes("`@clerk/react-router`") check that can falsely treat unrelated
mentions as already configured. Update the logic in the config inspection path
around the existing skip branch so it only skips after confirming
`@clerk/react-router` is actually present in the ssr.noExternal configuration
(preferably via the AST parse/inspection already used in this flow, or a much
narrower pattern tied to noExternal). If the match is only incidental, continue
with the normal wiring path and emit the manual Vite instruction instead of
returning skip from the current check.
1 participant
Summary
clerk initon a React Router 8 project scaffolds an app that is broken in dev out of the box: every request fails during SSR withuseNavigate() may be used only in the context of a <Router> component.This came out of root-causing a public field report where an agent couldn't one-shot a Clerk + React Router app.RR8 ships development/production conditional exports.
react-router devexternalizes@clerk/react-routerfor SSR, so Node resolves react-router's production build for Clerk while the app code gets the development build through Vite — two module instances, two Router contexts, and Clerk'suseNavigate()call insideClerkProviderthrows. Upstream: remix-run/react-router#15232. The scaffolder already handles the v7/v8 split for thev8_middlewareflag but never touched the vite config.Changes
ssr: { noExternal: ["@clerk/react-router"] }intovite.config.{ts,js,mts,mjs}whenreact-routeris v8+ (or unparseable, matching how thev8_middlewaregate already treats unknown versions). v7 has no conditional exports and is left alone.ssr.noExternalarray instead of clobbering it; skips when the config already references@clerk/react-router.Verification
create-react-router@latest→ RR 8.0.0 +@clerk/react-router@3.5.5): dev SSR 500s without the entry, HTTP 200 with it;react-router buildunaffected.bun run format,lint,typecheckclean;react-router.test.ts21/21 pass. (Fullbun run testhas 363 pre-existing failures in credential-store/host-execution on cleanmainin my sandbox — identical count with and without this change.)Not planned (follow-up candidates)
test/e2e/fixtures/react-router) pinsreact-router@7.15.0, so CI never exercises v8 — worth adding an RR8 fixture so the next regression of this class is caught.References
🤖 Generated with Claude Code