Skip to content

Commit faada87

Browse files
authored
Merge pull request #2199 from modelcontextprotocol/v2/chore/2196-consolidate-toolchain-devdeps
chore: consolidate shared toolchain devDependencies into the root package.json
2 parents 2eb160d + 440d1aa commit faada87

12 files changed

Lines changed: 882 additions & 6473 deletions

File tree

‎.claude/skills/local-dev/SKILL.md‎

Lines changed: 87 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -152,6 +152,93 @@ renderer with a React it was not installed against, which is the same split the
152152
pin exists to prevent, arrived at from the other side. `dedupe` still collapses
153153
each install to one copy, which is what actually has to hold.
154154

155+
### Why the shared toolchain is root-only
156+
157+
Every client's `validate` shells out to `prettier`, `eslint`, `tsc` and `vitest`,
158+
which makes them look like they belong beside the scripts that call them. They
159+
do not need to be: `npm run` prepends **every ancestor** `node_modules/.bin` to
160+
`PATH`, and Node and TypeScript walk parent `node_modules` /
161+
`node_modules/@types` the same way — so a client that declares no toolchain at
162+
all still resolves the root's copy. `clients/launcher` declares no
163+
`devDependencies` whatsoever and its `validate` is unchanged.
164+
165+
What a per-client declaration *does* buy is a second copy free to drift, and it
166+
had (#2196): `globals` sat at `^17.7.0` at the root against `^17.4.0` in all four
167+
clients, and `typescript-eslint` at `^8.65.0` against `^8.56.1`. Nothing failed —
168+
which is the point. A lint or format tool that differs per client makes the gate's
169+
verdict a function of *where you ran it*, and the exact `prettier` pin (#1790)
170+
only means something when there is one of it.
171+
172+
⚠️ The line is **used by every client**, not "used by one" and not "is it
173+
toolchain". `tsx`, `playwright`, `storybook`, `happy-dom`, `ink-testing-library`,
174+
`vite-node` and each client's own `@types/*` are toolchain too and stay where
175+
they are — hoisting them would make every client install the union of all four.
176+
So do the ones **more than one** client declares without all of them doing so:
177+
`tsup` sits in web, cli and tui, and `vite` in web and tui on top of the root
178+
*runtime* `dependency` that `--web --dev` needs. Neither is in scope here;
179+
whether to consolidate them is a separate call with a separate rationale (`vite`
180+
especially, since its root declaration is a `dependency`, not a
181+
`devDependency`).
182+
183+
#### What the walk-up does *not* buy you
184+
185+
⚠️ **Deleting a client's declaration does not always delete the copy** — and
186+
where a copy survives, it is the one that wins. Two mechanisms put one back,
187+
neither of which the manifest mentions:
188+
189+
- **An unmet peer.** npm auto-installs a peer into the install that needs it, and
190+
a client install has no visibility into the root's tree, so the root's copy
191+
cannot satisfy it. Web's `eslint-plugin-react-refresh` / `eslint-plugin-storybook`
192+
and the TUI's `eslint-plugin-react-hooks` each pull a client-local `eslint`;
193+
web's Storybook/Vitest stack pulls a local `typescript` and `vitest`.
194+
- **A hoisted transitive.** `@types/express` brings `@types/node` into web and
195+
cli's trees on its own.
196+
197+
Those copies sit *nearer* than the root's, so `clients/web/node_modules/.bin`
198+
precedes the root bin directory on `PATH` and TypeScript resolves the nearest
199+
`node_modules/@types`. Verify with `npm exec -- which eslint` from the client
200+
rather than assuming — the assumption is what made the first cut of #2196 claim
201+
more than it delivered (Copilot).
202+
203+
So the consolidation buys **one declaration and one place to bump**, not one copy
204+
on disk. The two mechanisms are **not** equally safe, and neither is a guarantee:
205+
206+
- A **peer** copy is at least constrained by its holder's peer range. That is a
207+
real pin only when the range is exact — `@vitest/browser-playwright` pins
208+
`vitest` to a single version, which is why the trio below is pinned too. For a
209+
wide range (`eslint-plugin-react-refresh` accepts `^9 || ^10`) the copies agree
210+
only because npm happens to resolve the same latest in both installs, which is
211+
a coincidence that holds until it doesn't.
212+
- A **transitive** copy is constrained by nothing of ours whatsoever, and one has
213+
already diverged: cli's `@types/node` is `24.13.1` against the root's
214+
`24.13.3`, and was `24.13.1` on `v2/main` too — a declared `^24.12.4` loses to
215+
a nearer transitive.
216+
217+
⚠️ **Nothing gates either of those, and `verify:dep-lockstep` is not it.**
218+
That guard derives its candidate set from what each `tsc` **program** resolves
219+
(see below), so it sees only packages a program loads from two installs. A tool
220+
*binary* — `eslint`, `prettier`, `vitest` — never enters a program, so it is
221+
outside the candidate set no matter how far it drifts, and the cli `@types/node`
222+
skew above passes for a second reason on top of that: no one program sees both
223+
copies. When you change what a client declares, check by hand from that client:
224+
225+
```sh
226+
cd clients/web && npm exec -- which eslint prettier tsc vitest
227+
```
228+
229+
#### Why the vitest trio is pinned exactly
230+
231+
`@vitest/browser-playwright` declares an **exact** peer on `vitest` (`"vitest":
232+
"4.1.10"`, not a range), so that package — not the root's range — decides which
233+
`vitest` lands in `clients/web`. Left floating, the root resolves the newest
234+
patch while web's peer stays pinned to the older one, and web's tests then run on
235+
one `vitest` while loading a `@vitest/coverage-v8` provider built against
236+
another. Both would still pass, which is the bad part.
237+
238+
So `vitest` and `@vitest/coverage-v8` at the root and `@vitest/browser-playwright`
239+
in `clients/web` are all pinned **exactly**, and a bump edits all three in one
240+
change — the same discipline as the exact `prettier` pin (#1790).
241+
155242
### Why runtime consumption decides `dependencies`
156243

157244
The client builds externalise npm packages, so a published install resolves them

‎AGENTS.md‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -90,6 +90,9 @@ The reasoning behind each of these, and what breaks when it is ignored, is the
9090
- **A client declares only what that client alone consumes** — its own UI stack, its bundler-inlined packages, its dev tooling. `clients/cli` and `clients/launcher` therefore declare **no** runtime dependencies at all, and that is the expected steady state, not an omission: everything they run on is root-declared and resolves by walk-up from the client directory. Re-adding a root-declared package to a client manifest re-creates the second copy this rule exists to make impossible (#1896), so a missing module at runtime is a signal to check the **root** manifest and the client's `external` list, never to add it back.
9191
- **A package that moves to the root moves its `vitest.shared.mts` pin with it.** Left pointing at `<client>/node_modules` a pin resolves to a directory that no longer exists — or, where a transitive copy happens to sit there (`chokidar` under `vite`, `react` as a peer of `react-dom` and `ink`), to the very duplicate the pin list exists to prevent. **`react` and `react-dom` are the deliberate exception** and stay pinned per client, so a client's renderer and the React it calls into come from one install; every other root-owned pin resolves from the repo root.
9292
- **`dependencies` vs `devDependencies` follows from who consumes it at runtime**, not from where it is declared. Anything `core/` imports at runtime must be a root **`dependency`** — the client builds externalize npm packages and a published install resolves them from the root manifest, where devDependencies are absent.
93+
- **The shared toolchain is declared once, at the repo root, and in no client manifest.** `eslint`, `@eslint/js`, `typescript-eslint`, `globals`, `prettier`, `typescript`, `vitest`, `@vitest/coverage-v8` and `@types/node` are used by every client's own scripts, and a client that declares none of them still resolves the root copy by walk-up — `npm run` puts each ancestor `node_modules/.bin` on `PATH`, and Node and TypeScript walk parent `node_modules` / `node_modules/@types` the same way. `clients/launcher` declares no `devDependencies` at all and its `validate` is unchanged. A client-side declaration buys nothing and installs a second copy free to drift, as `globals` (`^17.7.0` root / `^17.4.0` clients) and `typescript-eslint` (`^8.65.0` / `^8.56.1`) had before #2196. These stay **`devDependencies`** — none is consumed at runtime and the tarball ships only each client's `build/`. The boundary is **used by every client**, not "used by one": anything narrower stays where it is, whether one client declares it (`tsx`, `playwright`, `storybook`, `happy-dom`, `ink-testing-library`, `vite-node`, each client's own `@types/*`) or several do — `tsup` is declared in web, cli and tui, and `vite` in web and tui on top of the root **runtime** `dependency` that `--web --dev` needs. Those are out of scope here; consolidating them is a different call with a different rationale.
94+
- ⚠️ **Deleting the declaration does not always delete the copy, and the local copy still wins.** npm auto-installs an unmet **peer** into the install that needs it, and it has no visibility into the root's tree — so a client-only ESLint plugin drags a client-local `eslint` in (`eslint-plugin-react-refresh`/`-storybook` in web, `eslint-plugin-react-hooks` in tui), and web's Storybook/Vitest stack drags in a local `typescript` and `vitest`. A hoisted transitive does the same: `@types/express` puts an `@types/node` in web and cli. Those copies sit *nearer* than the root's and take precedence. The consolidation is therefore about **one declaration and one place to bump**, not about a single copy on disk. ⚠️ **Nothing keeps the surviving copies aligned, and nothing gates them.** A **peer** copy is at least constrained by its holder's peer range — tightly for `vitest` (an exact peer, hence the pin below), loosely for `eslint` (`^9 || ^10`), where the copies agree only because npm resolves the same latest in both installs. A **transitive** copy is constrained by nothing of ours at all, and cli's `@types/node` (`24.13.1` against the root's `24.13.3`) has already diverged on exactly that. `verify:dep-lockstep` does not catch either: it compares only packages that one `tsc` **program** loads from two installs, so a stray `eslint`, `prettier` or `vitest` binary is outside its candidate set entirely, and the cli `@types/node` difference goes unreported because no one program sees both copies. Check a tool copy by hand — `npm exec -- which eslint` from the client — when you change what a client declares.
95+
- ⚠️ **`vitest`, `@vitest/coverage-v8` and web's `@vitest/browser-playwright` are pinned exactly, and move together.** `@vitest/browser-playwright` declares an **exact** peer on `vitest`, so it — not the root range — decides which `vitest` web installs. Left to float, the root resolves a newer patch and web's tests then run on one `vitest` while loading a coverage provider built against another. Bumping means editing all three in one change, the same discipline the exact `prettier` pin (#1790) exists for.
9396
- **A root-declared package that `core/` imports at runtime must also be named in all three bundler `external` lists** (`clients/{cli,tui}/tsup.config.ts`, `clients/web/tsup.runner.config.ts`), since which client reaches it is a function of what `core/` imports rather than of what the client's own code names. `npm run verify:bundle-externals` enforces this against the **built output**.
9497
- **A dependency that renders React components must be bundled** into the client that uses it (`noExternal`) and declared only there — an externalized one resolves its own `react` and splits the tree. `ink` is the single exemption, on cost, and it is only safe while the root `react` range stays open to the whole major (`^19.0.0`).
9598
- **One version per install-crossing dependency.** When bumping a dependency the shared sources pull in, bump it in every install that declares it. Consolidating to the root is what makes most of these unbumpable in two places at once, but it does not retire the rule — a client's `devDependencies`, and any package that arrives transitively into a client install, can still skew against the root. Never raise the tsc heap to work around one. `npm run verify:dep-lockstep` enforces this.

0 commit comments

Comments
 (0)