fix(deps)!: move react-router to peerDependencies (^8.3.0) - #465
fix(deps)!: move react-router to peerDependencies (^8.3.0)#465interacsean wants to merge 2 commits into
Conversation
Code Metrics Report
Details | | main (fdf7e6a) | #465 (665928c) | +/- |
|---------------------|----------------|----------------|------|
| Coverage | 90.0% | 90.0% | 0.0% |
| Files | 126 | 126 | 0 |
| Lines | 5174 | 5174 | 0 |
| Covered | 4661 | 4661 | 0 |
- | Test Execution Time | 2m0s | 2m3s | +3s |Code coverage of files in pull request scope (0.0% → 0.0%)
Reported by octocov |
react-router carries React context, and context identity is per module instance, so app-shell and the app it powers must share one copy. As a regular dependency that was never guaranteed: whenever an app's resolved react-router differed from app-shell's, both were installed and the two routers were disjoint. The failure is asymmetric, which is why it went unnoticed: app-shell keeps working against its own copy — sidebar, breadcrumbs, navigation all fine — while the app's own useNavigate / useLocation / <Link> throw "may be used only in the context of a <Router> component". TypeScript cannot see it, so builds pass. It is also time-triggered rather than configuration-triggered: a new react-router release can shift one range's resolution and not the other's with no code change on either side. #412 addressed this by matching majors and documenting "import routing primitives from app-shell". A reviewer noted at the time that there were still no guardrails keeping an app's version in sync, and it was deferred. This is that guardrail. The range is a single major on purpose: app-shell builds and tests only against its ^8.3.0 devDependency, so a wider range would claim support CI never exercises. ^8.3.0 also sits above every published react-router advisory, preserving the alert-clearing outcome of #412. Also updates the routing docs, which previously told consumers not to install or import react-router — advice this change inverts. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
b295927 to
505ab04
Compare
|
Thanks for investigating this — I understand the duplicate-instance problem and why making My hesitation is more about the product/API boundary this creates. I do not want consumers of I feel similarly about the security-advisory motivation. If a consumer sees an advisory, uses AI or manual inspection to figure out that AppShell happens to use There is also a support-surface concern here. Once I’d be fine with leaving an escape hatch for advanced cases — for example, allowing consumers to pin or override AppShell’s transitive overrides:
"@tailor-platform/app-shell>react-router": "8.3.0"That kind of explicit opt-in escape hatch seems fine to me. What I want to avoid is making So while I agree that the duplicate-router issue is real, I’d prefer us to look for a solution that preserves the “AppShell owns routing” model if possible, rather than making |
|
Yes I suppose the issue this tries to fix is only valid if the consumer application has react-router installed as dependency to try to achieve some customization that is not possible with what app-shell re-exports. And adding peer-dependency nudges consumers that may not otherwise have RR as a dependency to install it. Some more thoughts after reading your comment... My first instinct to cover what you are getting at is to re-export everything that react router does, so that users would have no need to install RR in their dependencies, and struggle with keeping versions aligned. Just because we make it available, it does not mean that all consumers will realise this is how it needs to be done though, and some users may still install RR as direct-dependency, and then suffer version-mismatch issues when we update app-shell, or they update RR. If we do not re-export everything under the RR API, then users could always find a need to install and us RR directly. We won't be able to think of all the ways an AS consumer will want to manipulate routing. I feel there are some trade-offs either way. PeerDependencies encourages consumers to use RR — although, if we are assured the versions are aligned then maybe there is nothing too wrong with that. If they start using import from react-router, and what they import is compatible with the RR we use internally, one way of looking at it is that it is fine. Maybe the lowest friction for now is to re-export all the RR API from App-Shell, so we are not forcing any consumer to install RR to meet their requirements |
Closes tailor-inc/platform-planning#1721
Moves
react-routerfromdependenciestopeerDependenciesat^8.3.0, keeping it as adevDependencyfor app-shell's own build and tests. Plus amigrations.mdentry and a quickstart note.Draft — the semver call and the CI question below want a decision first.
Why
react-routercarries React context, and context identity is per module instance, so app-shell and the app it powers must share one copy. As a regular dependency that was never guaranteed: whenever an app's resolved version differed from app-shell's, both got installed and the two routers were disjoint.The failure is asymmetric, which is why it went unnoticed for so long: app-shell keeps working against its own copy — sidebar, breadcrumbs, navigation all fine — while the app's own
useNavigate/useLocation/<Link>throwmay be used only in the context of a <Router> component. TypeScript cannot see it, so builds pass. It is also time-triggered rather than configuration-triggered: a newreact-routerrelease can shift one range's resolution and not the other's, with no code change on either side.#412 addressed this by matching majors and documenting "import routing primitives from app-shell". A reviewer noted at the time that there were still no guardrails keeping an app's version in sync, and it was deferred. This is that guardrail — and it has since been reproduced on a real consumer at the exact version pair measured below.
Evidence
Measured in a faithful external consumer: a Vite app outside this monorepo installing app-shell from a
pnpm packtarball with its ownreact-router,AppShellmounted withbasePath="app"so basename handling is exercised too. This matters because the in-repo examples resolvereact-routerthrough a workspace link in a way no real consumer does — and the monorepo dev server is misleadingly forgiving, passing whilevite buildproduced two copies.dependencies, app on a different 8.xpeerDependencies, app on 8.xUnder peer deps the app's version always wins — app-shell stops choosing its own router version. Behavioural checks, not just absence of throws: location agreement across both import paths, basename resolution through consumer
<Link>,useParamson an app-shell route,setSearchParamsvisible to app-shell, app-shell'sredirectToguard firing on app-initiated navigation, chrome following app navigation,history.back()after mixed navigation — and consumeruseBlockerintercepting app-shell-initiated navigation, which is only possible on one shared router.Why not just document
resolve.dedupe: it does collapse the bundle, and works underdependenciestoo, but nothing obliges an app to set it, it must coverreact-routerandreact-router/dom, and it can bind app-shell to a major it was never compiled against — measured: with dedupe on and the app on react-router 7, app-shell built against 8.3.0 silently ran on 7.18.2.Impact by what an app declares today
react-routerimports fail at build until declared^8.3.0, or any range resolving to the same version8.2.0)ERESOLVE/ pnpm unmet-peer warning — needs the 7→8 upgrade in the same changeThe "declares nothing" row is the one most likely to surprise: worth grepping non-pinning frontends for direct
react-routerimports before release. Apps pinned to v7 are already broken on 1.11.0–1.12.0; the install message is how they find out.Security is unaffected: every published react-router advisory has a patch inside the v7 line and
^8.3.0sits above all of them, so the alert-clearing outcome of #412 is preserved. Peer deps also shrink app-shell's own alert surface, since react-router stops being a runtime dependency.Verification
pnpm build,pnpm type-check,pnpm lint,pnpm test(81 files, 1570 tests) pass. Both example apps build.Open questions
minor(1.13.0), provisional pending a team discussion. For minor: most apps already declare react-router 8, and both package managers auto-install when an app declares nothing. For major: it changes the install contract, and v7-pinned apps hit a hardERESOLVE. Note a minor means apps on^1.12.0pick this up on a routine update rather than opting in.react-routerexplicitly? They don't today and still build, since core's devDependency satisfies the peer through the workspace link. Declaring it would model the correct consumer pattern.🤖 Generated with Claude Code