docs: correct the dependency-classification claims
Copilot review round 2, four findings, all factual errors in the docs added by the previous commit: - `yaml` is in the root `dependencies`, not `devDependencies`, and its only importer is `test-servers/src/load-config.ts` — not `core/` or the root tooling, as the `vitest.shared.mts` comment claimed. - The rule as written told a reader to put anything reached through root-owned code — `core/` included — in `devDependencies`. That is wrong and would break the published package silently: the client builds externalize npm packages, so a published install resolves them from the root manifest, where devDependencies are absent. Placement (which manifest) and classification (which section) are now stated as separate questions, the second following from who consumes the package at runtime. `yaml`'s runtime classification is left alone and called out instead: moving it changes what ships, which is not a docs change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
This commit is contained in:
cliffhall
parent
e132d551e6
commit
5a3a3436ea
4 changed files with 20 additions and 13 deletions
3
.github/copilot-instructions.md
vendored
3
.github/copilot-instructions.md
vendored
|
|
@ -76,7 +76,8 @@ Both exist and do different jobs. Theme files (`src/theme/<Component>.ts`) custo
|
|||
|
||||
- **The MCP SDK packages (`@modelcontextprotocol/client`, `core`, `server`, `server-legacy`, `ext-apps`) belong in the root `package.json` only.** Adding one to a `clients/*/package.json` is a review finding: Node resolution walks up, so the root install already serves every client, and a per-client entry installs a second copy that drifts (it produced two versions of `ext-apps`, and of the transitive v1 SDK, at once — #1970) and reintroduces the duplicate-copy failure `vitest.shared.mts` has a `dedupe` workaround for.
|
||||
- **The v1 `@modelcontextprotocol/sdk` is not a dependency of this repo** and must not become one. It is a peer of `ext-apps`, present in lock files only.
|
||||
- Dependencies used only by **root-owned code with no manifest** (`test-servers/src`, `core/`) go in the root `devDependencies` and are aliased to the **repo root** in `vitest.shared.mts` — as `express` and `yaml` are — not to `<client>/node_modules` like the other pins there.
|
||||
- Dependencies reached only through **root-owned code with no manifest** (`test-servers/src`, `core/`) are declared at the root and aliased to the **repo root** in `vitest.shared.mts` — as `express` and `yaml` are — not to `<client>/node_modules` like the other pins there.
|
||||
- **Which section is a separate question from which manifest.** A package `core/` imports at runtime must be in root **`dependencies`**: client builds externalize npm packages, so a published install resolves them from the root manifest and devDependencies are absent there. Only test/build-only packages (`express`) belong in `devDependencies`. Flag a runtime `core/` import added to `devDependencies` — it passes every local check and breaks the published package.
|
||||
|
||||
## Tests and the coverage gate
|
||||
|
||||
|
|
|
|||
|
|
@ -117,7 +117,13 @@ After installing, `npm run build` builds all clients. The launcher scripts (`npm
|
|||
|
||||
**The MCP SDK packages — `@modelcontextprotocol/client`, `core`, `server`, `server-legacy`, `ext-apps` — are declared in the repo-root `package.json` and nowhere else.** Do not add them to a `clients/*/package.json`: Node resolution walks up, so the root install already serves every client, and a per-client declaration installs a *second copy* that drifts from the root's. That drift is not theoretical — it put two versions of `ext-apps` (1.7.4 / 1.7.5), and of the transitive v1 `@modelcontextprotocol/sdk` (1.29.0 / 1.30.0), in the tree at once (#1970), and a second copy of `client`/`core` is exactly the failure the `dedupe` + `server.deps.inline` workaround in `vitest.shared.mts` exists for.
|
||||
|
||||
The same rule covers anything reached only through **root-owned code that has no manifest of its own**: `express` (imported by `test-servers/src`) and `yaml` are root devDependencies, and `vitest.shared.mts` aliases both to the **repo root** rather than to `<client>/node_modules` like its other pins. If you add a dependency to `test-servers/src` or `core/`, declare it at the root and alias it there too.
|
||||
The same **placement** rule covers anything reached only through **root-owned code that has no manifest of its own** (`test-servers/src`, `core/`): declare it at the root, and alias it to the **repo root** in `vitest.shared.mts` rather than to `<client>/node_modules` like that file's other pins. `express` and `yaml` are the two today — both reached through `test-servers/src` (express by the http/oauth servers, yaml by `load-config.ts`).
|
||||
|
||||
**Placement and classification are separate questions.** Which *section* of the root manifest it goes in follows from who consumes it at runtime:
|
||||
|
||||
- A package `core/` imports at runtime belongs in root **`dependencies`**. The client builds bundle `core/` but externalize npm packages, so a published install resolves them from the root manifest — and devDependencies are not installed for consumers, so a runtime import parked there breaks the published package while passing every local check.
|
||||
- A package only the tests, the test servers, or the build tooling need belongs in **`devDependencies`** — `express`, added there by #1970.
|
||||
- `yaml` is in `dependencies` today even though its only importer is `test-servers/src/load-config.ts`. Left as-is deliberately (moving it changes what ships, which is not a docs change); if you touch it, confirm no published path reads YAML first.
|
||||
|
||||
The v1 SDK (`@modelcontextprotocol/sdk`) is **not** a dependency of this repo and must not become one — v2 uses the packages above. It appears in the lock files only as a `"peer": true` entry pulled in by `ext-apps`.
|
||||
|
||||
|
|
|
|||
|
|
@ -66,7 +66,7 @@ npm install # root install; postinstall cascades into every client
|
|||
|
||||
The cascade (`scripts/install-clients.mjs`) is dev-only — it exits early when the package is installed as a dependency, and the published tarball ships only each client's `build/`, so end users are unaffected. Set `INSPECTOR_SKIP_CLIENT_INSTALL=1` to skip it.
|
||||
|
||||
**Where a dependency is declared.** The MCP SDK packages (`@modelcontextprotocol/client`, `core`, `server`, `server-legacy`, `ext-apps`) live in the **root** `package.json` only — never in a client's. Node resolution walks up, so the root install is on every client's chain, and the root manifest is already what the published tarball resolves against. Declaring them per client installs a second copy that can drift from the root's, which is how two versions of `ext-apps` (and of the transitive v1 `@modelcontextprotocol/sdk`) ended up in the tree before [#1970](https://github.com/modelcontextprotocol/inspector/issues/1970) — and a second copy of `client`/`core` is the failure `vitest.shared.mts` carries a `dedupe` workaround for. The same holds for anything used only by root-owned code with no manifest of its own: `express` (imported by `test-servers/src`) and `yaml` are root devDependencies, resolved from the root by `vitest.shared.mts`.
|
||||
**Where a dependency is declared.** The MCP SDK packages (`@modelcontextprotocol/client`, `core`, `server`, `server-legacy`, `ext-apps`) live in the **root** `package.json` only — never in a client's. Node resolution walks up, so the root install is on every client's chain, and the root manifest is already what the published tarball resolves against. Declaring them per client installs a second copy that can drift from the root's, which is how two versions of `ext-apps` (and of the transitive v1 `@modelcontextprotocol/sdk`) ended up in the tree before [#1970](https://github.com/modelcontextprotocol/inspector/issues/1970) — and a second copy of `client`/`core` is the failure `vitest.shared.mts` carries a `dedupe` workaround for. The same root-only placement holds for anything reached solely through root-owned code with no manifest of its own (`test-servers/src`, `core/`), and `vitest.shared.mts` aliases those to the repo root — `express` and `yaml`, both reached through `test-servers/src`, are the two today. **Whether such a package is a `dependency` or a `devDependency` follows from who consumes it at runtime, not from where it is declared:** anything `core/` imports at runtime must be a root **`dependency`**, because the client builds externalize npm packages and a published install resolves them from the root manifest, where devDependencies are absent. `express` is test-only and is a devDependency; `yaml` currently sits in `dependencies`.
|
||||
|
||||
## Running during development
|
||||
|
||||
|
|
|
|||
|
|
@ -83,16 +83,16 @@ export function vitestSharedPaths(clientDir: string) {
|
|||
replacement: path.resolve(dirname, "node_modules/@napi-rs/keyring"),
|
||||
},
|
||||
// `express` and `yaml` resolve from the **repo root**, unlike every pin
|
||||
// above. Both are reached only through root-owned code that has no manifest
|
||||
// of its own — `test-servers/src` imports express, and `core`/the root
|
||||
// tooling uses yaml — so the root is where they are declared and the only
|
||||
// place a client's resolution chain is guaranteed to find them. Pointing
|
||||
// these at `<client>/node_modules` is what broke when the MCP packages moved
|
||||
// to the root (#1970): express was never declared by a client at all, it
|
||||
// arrived in `clients/cli` as a peer of `express-rate-limit` under
|
||||
// `@modelcontextprotocol/server-legacy`, so removing that manifest entry
|
||||
// took express with it and every cli test that spawns a test server failed
|
||||
// to resolve it.
|
||||
// above. Both are reached only through `test-servers/src` — express by the
|
||||
// http/oauth servers, yaml by `load-config.ts` — which is root-owned code
|
||||
// with no manifest of its own, so the root is where they are declared and
|
||||
// the only place a client's resolution chain is guaranteed to find them.
|
||||
// Pointing these at `<client>/node_modules` is what broke when the MCP
|
||||
// packages moved to the root (#1970): express was never declared by a client
|
||||
// at all, it arrived in `clients/cli` as a peer of `express-rate-limit`
|
||||
// under `@modelcontextprotocol/server-legacy`, so removing that manifest
|
||||
// entry took express with it and every cli test that spawns a test server
|
||||
// failed to resolve it.
|
||||
{
|
||||
find: /^express$/,
|
||||
replacement: path.resolve(repoRoot, "node_modules/express"),
|
||||
|
|
|
|||
Loading…
Reference in a new issue