chore: address Copilot review round 3 on #2200
Two suppressed comments, both accurate. - Pin zod, zod/v4 and open to the root in vitest.shared.mts, and add matching paths to the three clients/web tsconfigs. The claim they replace — that these resolve unambiguously by walk-up — was wrong: both still sit at the top level of clients/web/node_modules as transitive copies (zod under eslint-plugin-react-hooks, open under Storybook), so an unpinned bare import from a web test resolved the client copy while core/ and the SDK packages resolved the root's. For open that is only wasteful; for zod it is a schema built by one copy and an instanceof check made by the other across the whole @modelcontextprotocol/* surface. zod is deduped as well, and zod/v4 gets its own entry because first-party code imports both specifiers and pinning one would split the package rather than collapse it. - Update the consumer comments that #2195 falsified. clients/web/vite.config.ts (three places) and server/start-vite-dev-server.ts all still said the shared aliases resolve pino/hono and friends from clients/web/node_modules; they now state the split — react and react-dom from the client install, everything root-declared from the repo root. - local-dev skill: replace the wrong 'resolves unambiguously' list with the distinction that actually matters. Deleting a declaration is not the same as the package resolving from the root, because chokidar, zod, open and react all sit in a client install transitively; check the install, not the manifest. ajv, commander and undici carry no pin because no client install has a copy. npm run local:gate passes from a cold tsc build. Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall
parent
2a64c6716b
commit
566990382a
7 changed files with 70 additions and 18 deletions
|
|
@ -111,10 +111,16 @@ Keep two distinctions straight, because AGENTS.md's rules split on them:
|
|||
`external` lists.
|
||||
- **Root-declared is not the same as aliased.** The `vitest.shared.mts` pins and
|
||||
the `clients/web/tsconfig.*.json` `paths` cover the packages whose resolution
|
||||
is genuinely ambiguous — a transitive copy sits in the client install, or the
|
||||
importer is `core/`, which has no manifest to resolve from. A package that
|
||||
resolves unambiguously by walk-up (`ajv`, `commander`, `open`, `undici`,
|
||||
`zod`) is deliberately absent from both.
|
||||
is genuinely ambiguous, which is two different situations: the importer is
|
||||
`core/`, which has no manifest to resolve from, **or** a copy still sits in
|
||||
the client install and would win. The second case is the one that is easy to
|
||||
get wrong — `chokidar` (under `vite`), `zod` (under
|
||||
`eslint-plugin-react-hooks`), `open` (under Storybook) and `react` (a peer of
|
||||
`react-dom` and `ink`) are all present in a client's `node_modules` without
|
||||
being declared there, so "we deleted the declaration" is not the same as "it
|
||||
resolves from the root now". **Check the install, don't reason from the
|
||||
manifest.** `ajv`, `commander` and `undici` carry no pin because no client
|
||||
install has a copy of them at all.
|
||||
|
||||
The point of deleting the client-side copies rather than merely keeping them in
|
||||
step is that **a package installs only into an install root that declares it**.
|
||||
|
|
|
|||
|
|
@ -36,8 +36,9 @@ export async function startViteDevServer(
|
|||
// `server.fs.allow` here. Without them, App.tsx's `@inspector/core/*`
|
||||
// imports fail to resolve and the page 500s (#1452 smoke test). The aliases
|
||||
// and dedupe are factored into `vitest.shared.mts` so both paths stay in
|
||||
// sync — pass the client dir (`root`) so bare-module pins resolve against
|
||||
// `clients/web/node_modules`.
|
||||
// sync — pass the client dir (`root`), which is what the `react` /
|
||||
// `react-dom` pins resolve against. The root-declared packages resolve from
|
||||
// the repo root instead, which that file derives from the same argument.
|
||||
const { repoRoot, sharedAliases, sharedDedupe, nodeModulesAliases } =
|
||||
vitestSharedPaths(root);
|
||||
const inlineConfig: InlineConfig = {
|
||||
|
|
|
|||
|
|
@ -57,6 +57,9 @@
|
|||
"atomically": ["../../node_modules/atomically"],
|
||||
"chokidar": ["../../node_modules/chokidar"],
|
||||
"@napi-rs/keyring": ["../../node_modules/@napi-rs/keyring"],
|
||||
"zod": ["../../node_modules/zod"],
|
||||
"zod/v4": ["../../node_modules/zod/v4"],
|
||||
"open": ["../../node_modules/open"],
|
||||
"react-syntax-highlighter/dist/esm/prism-light": [
|
||||
"./src/types/react-syntax-highlighter-prism-light"
|
||||
],
|
||||
|
|
|
|||
|
|
@ -55,7 +55,10 @@
|
|||
"@hono/node-server": ["../../node_modules/@hono/node-server"],
|
||||
"atomically": ["../../node_modules/atomically"],
|
||||
"chokidar": ["../../node_modules/chokidar"],
|
||||
"@napi-rs/keyring": ["../../node_modules/@napi-rs/keyring"]
|
||||
"@napi-rs/keyring": ["../../node_modules/@napi-rs/keyring"],
|
||||
"zod": ["../../node_modules/zod"],
|
||||
"zod/v4": ["../../node_modules/zod/v4"],
|
||||
"open": ["../../node_modules/open"]
|
||||
}
|
||||
},
|
||||
"include": ["vite.config.ts", "tsup.runner.config.ts", "server/**/*.ts"]
|
||||
|
|
|
|||
|
|
@ -21,6 +21,9 @@
|
|||
"express": ["./node_modules/@types/express"],
|
||||
"yaml": ["./node_modules/yaml"],
|
||||
"@napi-rs/keyring": ["../../node_modules/@napi-rs/keyring"],
|
||||
"zod": ["../../node_modules/zod"],
|
||||
"zod/v4": ["../../node_modules/zod/v4"],
|
||||
"open": ["../../node_modules/open"],
|
||||
"react-syntax-highlighter/dist/esm/prism-light": [
|
||||
"./src/types/react-syntax-highlighter-prism-light"
|
||||
],
|
||||
|
|
|
|||
|
|
@ -143,8 +143,12 @@ export default defineConfig(({ command }) => {
|
|||
// (#1244), the browser dep graph reached bare-module subpaths in core/
|
||||
// that Rolldown couldn't resolve against `core/`'s parent (it has no
|
||||
// node_modules of its own). Promote the same bare-module aliases the
|
||||
// vitest projects use so `vite dev` / `vite build` can resolve them
|
||||
// from `clients/web/node_modules`.
|
||||
// vitest projects use so `vite dev` / `vite build` can resolve them.
|
||||
//
|
||||
// Which install each alias points at is `vitest.shared.mts`'s decision,
|
||||
// not this file's, and since #2195 it is no longer one answer: `react`
|
||||
// and `react-dom` resolve from `clients/web/node_modules`, everything
|
||||
// root-declared resolves from the repo root.
|
||||
alias: [
|
||||
...Object.entries(sharedAliases).map(([find, replacement]) => ({
|
||||
find,
|
||||
|
|
@ -153,9 +157,9 @@ export default defineConfig(({ command }) => {
|
|||
...nodeModulesAliases,
|
||||
],
|
||||
// Source files in core/ import bare modules (react, @testing-library/react,
|
||||
// etc.) that only exist in clients/web/node_modules. Dedupe ensures Vite
|
||||
// resolves them from this package rather than walking up from core/'s
|
||||
// location (which has no node_modules of its own yet).
|
||||
// etc.) that core/ cannot resolve for itself, having no node_modules of
|
||||
// its own. Dedupe collapses each of these to a single copy per install,
|
||||
// whichever install the alias above selected.
|
||||
dedupe: sharedDedupe,
|
||||
},
|
||||
// Pin the Vite dev server to the same port and host the Hono plugin
|
||||
|
|
@ -274,10 +278,11 @@ export default defineConfig(({ command }) => {
|
|||
{
|
||||
extends: true,
|
||||
// Vitest projects don't inherit `resolve` from the parent. The unit
|
||||
// project runs from repoRoot (so vitest's coverage transformer can
|
||||
// reach core/), but repoRoot has no node_modules of its own — the
|
||||
// shared regex aliases redirect bare `react`/`pino`/etc. imports
|
||||
// from core/ back into clients/web/node_modules.
|
||||
// project runs from repoRoot so vitest's coverage transformer can
|
||||
// reach core/, which leaves core/'s own bare imports with nothing to
|
||||
// resolve against — the shared regex aliases are what answer them,
|
||||
// sending `react` to clients/web/node_modules and the root-declared
|
||||
// packages (`pino`, `hono`, …) to the repo root (#2195).
|
||||
resolve: projectResolve,
|
||||
test: {
|
||||
name: "unit",
|
||||
|
|
@ -331,8 +336,9 @@ export default defineConfig(({ command }) => {
|
|||
extends: true,
|
||||
// See note on the unit project: integration tests also run from
|
||||
// repoRoot and import core/ modules, so they need the same alias
|
||||
// setup. The shared bare-module aliases keep `pino`, `hono`, etc.
|
||||
// resolving against clients/web/node_modules.
|
||||
// setup, with the same split — `react` from this client's install,
|
||||
// `pino` / `hono` and the rest of the root-declared set from the
|
||||
// repo root.
|
||||
resolve: projectResolve,
|
||||
test: {
|
||||
name: "integration",
|
||||
|
|
|
|||
|
|
@ -24,6 +24,10 @@ export function vitestSharedPaths(clientDir: string) {
|
|||
"react-dom",
|
||||
"@modelcontextprotocol/client",
|
||||
"@modelcontextprotocol/core",
|
||||
// Every SDK schema is a zod type, so a second copy breaks `instanceof`
|
||||
// across the whole surface. The alias below picks the install; this keeps
|
||||
// it to one copy within it.
|
||||
"zod",
|
||||
];
|
||||
|
||||
const nodeModulesAliases = [
|
||||
|
|
@ -131,6 +135,32 @@ export function vitestSharedPaths(clientDir: string) {
|
|||
find: /^@napi-rs\/keyring$/,
|
||||
replacement: path.resolve(repoRoot, "node_modules/@napi-rs/keyring"),
|
||||
},
|
||||
// `zod` and `open` are pinned for a different reason from everything above:
|
||||
// they resolve *somewhere* without help, and the somewhere is wrong. Both
|
||||
// still sit at the top level of `clients/web/node_modules` as transitive
|
||||
// copies — zod under `eslint-plugin-react-hooks`, open under Storybook —
|
||||
// so an unpinned bare import from a web test resolves the client copy while
|
||||
// `core/` and the SDK packages resolve the root's (Copilot).
|
||||
//
|
||||
// For `open` that is merely wasteful. For `zod` it is the hazard this file
|
||||
// exists for: two copies in one process means a schema built by one and an
|
||||
// `instanceof` check made by the other, across the entire
|
||||
// `@modelcontextprotocol/*` surface. The versions are identical today and
|
||||
// `verify:dep-lockstep` is what keeps them that way, but identical is not
|
||||
// the same as single, and only a pin makes it single.
|
||||
//
|
||||
// `zod/v4` needs its own entry — first-party code imports both specifiers,
|
||||
// and pinning only the bare one would split the package across two installs
|
||||
// rather than collapse it.
|
||||
{ find: /^zod$/, replacement: path.resolve(repoRoot, "node_modules/zod") },
|
||||
{
|
||||
find: /^zod\/v4$/,
|
||||
replacement: path.resolve(repoRoot, "node_modules/zod/v4/index.js"),
|
||||
},
|
||||
{
|
||||
find: /^open$/,
|
||||
replacement: path.resolve(repoRoot, "node_modules/open"),
|
||||
},
|
||||
];
|
||||
|
||||
const projectResolve = {
|
||||
|
|
|
|||
Loading…
Reference in a new issue