docs: correct the scope claims in the toolchain rule
Copilot review round 2. Docs only; no manifest or lockfile change. Two claims were wrong: - "A tool one client runs stays in that client" is contradicted by the manifests — 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. The boundary is "used by every client", not "used by one"; anything narrower stays put whether one client declares it or several do. Both are out of scope here and say so. - verify:dep-lockstep was named as the guard that the surviving copies agree. It is not: it derives its candidate set from what each tsc program resolves, so a tool binary never enters a program and is outside the set no matter how far it drifts — and cli's @types/node 24.13.1 against the root's 24.13.3 is a live skew that passes for the same reason. Both files now state the narrower guarantee, say that nothing gates the binaries automatically, note that each surviving copy is pinned by its holder's peer range, and give `npm exec -- which` as the manual check. Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall
parent
edef659fd9
commit
613c4949a4
2 changed files with 27 additions and 9 deletions
|
|
@ -118,11 +118,16 @@ which is the point. A lint or format tool that differs per client makes the gate
|
|||
verdict a function of *where you ran it*, and the exact `prettier` pin (#1790)
|
||||
only means something when there is one of it.
|
||||
|
||||
⚠️ The line is **who uses it**, not what kind of thing it is. `tsup`, `tsx`,
|
||||
`vite`, `vite-node`, `playwright`, `storybook`, `happy-dom`,
|
||||
`ink-testing-library` and each client's own `@types/*` are toolchain too and stay
|
||||
where they are, because only one client runs them. Hoisting those would make
|
||||
every client install the union of all four.
|
||||
⚠️ The line is **used by every client**, not "used by one" and not "is it
|
||||
toolchain". `tsx`, `playwright`, `storybook`, `happy-dom`, `ink-testing-library`,
|
||||
`vite-node` and each client's own `@types/*` are toolchain too and stay where
|
||||
they are — hoisting them would make every client install the union of all four.
|
||||
So do the ones **more than one** client declares without all of them doing so:
|
||||
`tsup` sits in web, cli and tui, and `vite` in web and tui on top of the root
|
||||
*runtime* `dependency` that `--web --dev` needs. Neither is in scope here;
|
||||
whether to consolidate them is a separate call with a separate rationale (`vite`
|
||||
especially, since its root declaration is a `dependency`, not a
|
||||
`devDependency`).
|
||||
|
||||
#### What the walk-up does *not* buy you
|
||||
|
||||
|
|
@ -145,8 +150,21 @@ rather than assuming — the assumption is what made the first cut of #2196 clai
|
|||
more than it delivered (Copilot).
|
||||
|
||||
So the consolidation buys **one declaration and one place to bump**, not one copy
|
||||
on disk. What has to hold is that the copies *agree*, and
|
||||
`npm run verify:dep-lockstep` is the guard for exactly that (see below).
|
||||
on disk. Each surviving copy is pinned by its holder's peer range rather than by
|
||||
a second declaration of ours, which is why they agree today.
|
||||
|
||||
⚠️ **Nothing gates that automatically, and `verify:dep-lockstep` is not it.**
|
||||
That guard derives its candidate set from what each `tsc` **program** resolves
|
||||
(see below), so it sees only packages a program loads from two installs. A tool
|
||||
*binary* — `eslint`, `prettier`, `vitest` — never enters a program, so it is
|
||||
outside the candidate set no matter how far it drifts, and cli's `@types/node`
|
||||
`24.13.1` against the root's `24.13.3` is a live skew that passes for the same
|
||||
reason (no one program sees both). When you change what a client declares, check
|
||||
by hand from that client:
|
||||
|
||||
```sh
|
||||
cd clients/web && npm exec -- which eslint prettier tsc vitest
|
||||
```
|
||||
|
||||
#### Why the vitest trio is pinned exactly
|
||||
|
||||
|
|
|
|||
|
|
@ -87,8 +87,8 @@ The reasoning behind each of these, and what breaks when it is ignored, is the
|
|||
|
||||
- **The MCP SDK packages** — `@modelcontextprotocol/client`, `core`, `server`, `server-legacy`, `ext-apps` — **are declared in the repo-root `package.json` and nowhere else.** So is anything reached only through root-owned code with no manifest of its own (`test-servers/src`, `core/`), aliased to the repo root in `vitest.shared.mts`. The v1 SDK (`@modelcontextprotocol/sdk`) is **not** a dependency of this repo and must not become one.
|
||||
- **`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.
|
||||
- **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/`. A tool **one** client runs stays in that client (`tsup`, `tsx`, `vite`, `vite-node`, `playwright`, `storybook`, `happy-dom`, `ink-testing-library`, and the `@types/*` for its own libraries).
|
||||
- ⚠️ **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; what must hold is that the copies **agree**, and `npm run verify:dep-lockstep` is the guard that says so.
|
||||
- **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.
|
||||
- ⚠️ **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. What keeps the surviving copies aligned is that each is pinned by its holder's peer range rather than by a second declaration of ours — and **nothing gates it automatically**. ⚠️ `verify:dep-lockstep` is *not* that gate: 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 `@types/node` patch difference between cli (`24.13.1`) and the root (`24.13.3`) is real and unreported for exactly that reason. Check a tool copy by hand — `npm exec -- which eslint` from the client — when you change what a client declares.
|
||||
- ⚠️ **`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.
|
||||
- **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**.
|
||||
- **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`).
|
||||
|
|
|
|||
Loading…
Reference in a new issue