chore: address Copilot review round 2 on #2200
- clients/web/tsconfig.{app,node,test}.json: repoint the pino, hono,
hono/streaming, @hono/node-server, atomically, chokidar and @napi-rs/keyring
path mappings from ./node_modules to ../../node_modules. The stale mappings
were not failing the build — an unresolvable paths entry falls back to
ordinary walk-up resolution, which finds the root copy — but chokidar is the
case where the fallback never runs: Vite's transitive copy still sits under
clients/web, so tsc was type-checking against the client's copy while Vitest
and the shipped bundle both resolved the root's. Same version today, silent
skew tomorrow. Both path prefixes now carry a comment saying which ownership
each one asserts.
- local-dev skill: the consolidation rationale no longer groups commander, open
and @hono/node-server (client-imported) with the core/ set, no longer claims
#2195 moved undici (it was already root-only), and no longer implies every
root-declared package has an alias.
npm run local:gate passes with clients/web/node_modules/.tmp cleared, so the
tsc results are from a cold build rather than an incremental cache.
Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
parent
f62cfbaeeb
commit
2a64c6716b
4 changed files with 58 additions and 26 deletions
|
|
@ -98,11 +98,23 @@ transitive v1 `@modelcontextprotocol/sdk` (1.29.0 / 1.30.0) in the tree at once
|
||||||
`server.deps.inline` workaround in `vitest.shared.mts` exists for.
|
`server.deps.inline` workaround in `vitest.shared.mts` exists for.
|
||||||
|
|
||||||
The same reasoning extends to anything reached only through root-owned code that
|
The same reasoning extends to anything reached only through root-owned code that
|
||||||
has no manifest of its own (`test-servers/src`, `core/`) — hence the repo-root
|
has no manifest of its own (`test-servers/src`, `core/`). #2195 made that the
|
||||||
alias for those in `vitest.shared.mts`. That used to be `express` and `yaml`
|
general case: `ajv`, `atomically`, `chokidar`, `hono`, `@napi-rs/keyring`,
|
||||||
alone; #2195 made it the general case, moving `ajv`, `atomically`, `chokidar`,
|
`pino`, `react` and `zod` moved to the root, joining `proper-lockfile` and
|
||||||
`commander`, `hono`, `@hono/node-server`, `@napi-rs/keyring`, `open`, `pino`,
|
`undici`, which were already there.
|
||||||
`react`, `undici` and `zod` to the root as well.
|
|
||||||
|
Keep two distinctions straight, because AGENTS.md's rules split on them:
|
||||||
|
|
||||||
|
- **Root-declared is not the same as `core/`-imported.** `commander`, `open` and
|
||||||
|
`@hono/node-server` are root `dependencies` too, but they are reached only
|
||||||
|
from client code. Only the `core/` set has to appear in *all three* bundler
|
||||||
|
`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.
|
||||||
|
|
||||||
The point of deleting the client-side copies rather than merely keeping them in
|
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**.
|
step is that **a package installs only into an install root that declares it**.
|
||||||
|
|
|
||||||
|
|
@ -30,6 +30,16 @@
|
||||||
* imports type-check; Vite's `dedupe` handles the runtime resolution —
|
* imports type-check; Vite's `dedupe` handles the runtime resolution —
|
||||||
* see vite.config.ts.
|
* see vite.config.ts.
|
||||||
*
|
*
|
||||||
|
* Two prefixes, and the difference is ownership rather than style. A
|
||||||
|
* `./node_modules/...` mapping names a package this client declares — the
|
||||||
|
* `@types/*` packages, `vitest`, `yaml`. A `../../node_modules/...` one
|
||||||
|
* names a package declared only in the root manifest (#2195), which is
|
||||||
|
* every runtime dependency `core/` imports. Those must be spelled out:
|
||||||
|
* `chokidar` in particular is still present under `clients/web` as a
|
||||||
|
* transitive copy of Vite's, so a `./node_modules` mapping resolves the
|
||||||
|
* client's copy at compile time while Vitest and the shipped bundle both
|
||||||
|
* resolve the root's. Same version today, and a silent skew tomorrow.
|
||||||
|
*
|
||||||
* Paths are relative to this config file (no `baseUrl` — deprecated in
|
* Paths are relative to this config file (no `baseUrl` — deprecated in
|
||||||
* TypeScript 6 / removed in 7).
|
* TypeScript 6 / removed in 7).
|
||||||
*/
|
*/
|
||||||
|
|
@ -38,15 +48,15 @@
|
||||||
"react": ["./node_modules/@types/react"],
|
"react": ["./node_modules/@types/react"],
|
||||||
"react-dom": ["./node_modules/@types/react-dom"],
|
"react-dom": ["./node_modules/@types/react-dom"],
|
||||||
"react/jsx-runtime": ["./node_modules/@types/react/jsx-runtime"],
|
"react/jsx-runtime": ["./node_modules/@types/react/jsx-runtime"],
|
||||||
"pino": ["./node_modules/pino"],
|
"pino": ["../../node_modules/pino"],
|
||||||
"hono": ["./node_modules/hono"],
|
"hono": ["../../node_modules/hono"],
|
||||||
"hono/streaming": [
|
"hono/streaming": [
|
||||||
"./node_modules/hono/dist/types/helper/streaming/index"
|
"../../node_modules/hono/dist/types/helper/streaming/index"
|
||||||
],
|
],
|
||||||
"@hono/node-server": ["./node_modules/@hono/node-server"],
|
"@hono/node-server": ["../../node_modules/@hono/node-server"],
|
||||||
"atomically": ["./node_modules/atomically"],
|
"atomically": ["../../node_modules/atomically"],
|
||||||
"chokidar": ["./node_modules/chokidar"],
|
"chokidar": ["../../node_modules/chokidar"],
|
||||||
"@napi-rs/keyring": ["./node_modules/@napi-rs/keyring"],
|
"@napi-rs/keyring": ["../../node_modules/@napi-rs/keyring"],
|
||||||
"react-syntax-highlighter/dist/esm/prism-light": [
|
"react-syntax-highlighter/dist/esm/prism-light": [
|
||||||
"./src/types/react-syntax-highlighter-prism-light"
|
"./src/types/react-syntax-highlighter-prism-light"
|
||||||
],
|
],
|
||||||
|
|
|
||||||
|
|
@ -32,20 +32,30 @@
|
||||||
* tsc follows those transitive imports while type-checking vite.config.ts,
|
* tsc follows those transitive imports while type-checking vite.config.ts,
|
||||||
* so this tsconfig needs the same alias tsconfig.app.json has.
|
* so this tsconfig needs the same alias tsconfig.app.json has.
|
||||||
*
|
*
|
||||||
|
* Two prefixes, and the difference is ownership rather than style. A
|
||||||
|
* `./node_modules/...` mapping names a package this client declares — the
|
||||||
|
* `@types/*` packages, `vitest`, `yaml`. A `../../node_modules/...` one
|
||||||
|
* names a package declared only in the root manifest (#2195), which is
|
||||||
|
* every runtime dependency `core/` imports. Those must be spelled out:
|
||||||
|
* `chokidar` in particular is still present under `clients/web` as a
|
||||||
|
* transitive copy of Vite's, so a `./node_modules` mapping resolves the
|
||||||
|
* client's copy at compile time while Vitest and the shipped bundle both
|
||||||
|
* resolve the root's. Same version today, and a silent skew tomorrow.
|
||||||
|
*
|
||||||
* Paths are relative to this config file (no `baseUrl` — deprecated in
|
* Paths are relative to this config file (no `baseUrl` — deprecated in
|
||||||
* TypeScript 6 / removed in 7).
|
* TypeScript 6 / removed in 7).
|
||||||
*/
|
*/
|
||||||
"paths": {
|
"paths": {
|
||||||
"@inspector/core/*": ["../../core/*"],
|
"@inspector/core/*": ["../../core/*"],
|
||||||
"pino": ["./node_modules/pino"],
|
"pino": ["../../node_modules/pino"],
|
||||||
"hono": ["./node_modules/hono"],
|
"hono": ["../../node_modules/hono"],
|
||||||
"hono/streaming": [
|
"hono/streaming": [
|
||||||
"./node_modules/hono/dist/types/helper/streaming/index"
|
"../../node_modules/hono/dist/types/helper/streaming/index"
|
||||||
],
|
],
|
||||||
"@hono/node-server": ["./node_modules/@hono/node-server"],
|
"@hono/node-server": ["../../node_modules/@hono/node-server"],
|
||||||
"atomically": ["./node_modules/atomically"],
|
"atomically": ["../../node_modules/atomically"],
|
||||||
"chokidar": ["./node_modules/chokidar"],
|
"chokidar": ["../../node_modules/chokidar"],
|
||||||
"@napi-rs/keyring": ["./node_modules/@napi-rs/keyring"]
|
"@napi-rs/keyring": ["../../node_modules/@napi-rs/keyring"]
|
||||||
}
|
}
|
||||||
},
|
},
|
||||||
"include": ["vite.config.ts", "tsup.runner.config.ts", "server/**/*.ts"]
|
"include": ["vite.config.ts", "tsup.runner.config.ts", "server/**/*.ts"]
|
||||||
|
|
|
||||||
|
|
@ -10,17 +10,17 @@
|
||||||
"react-dom": ["./node_modules/@types/react-dom"],
|
"react-dom": ["./node_modules/@types/react-dom"],
|
||||||
"react/jsx-runtime": ["./node_modules/@types/react/jsx-runtime"],
|
"react/jsx-runtime": ["./node_modules/@types/react/jsx-runtime"],
|
||||||
"vitest": ["./node_modules/vitest"],
|
"vitest": ["./node_modules/vitest"],
|
||||||
"pino": ["./node_modules/pino"],
|
"pino": ["../../node_modules/pino"],
|
||||||
"hono": ["./node_modules/hono"],
|
"hono": ["../../node_modules/hono"],
|
||||||
"hono/streaming": [
|
"hono/streaming": [
|
||||||
"./node_modules/hono/dist/types/helper/streaming/index"
|
"../../node_modules/hono/dist/types/helper/streaming/index"
|
||||||
],
|
],
|
||||||
"@hono/node-server": ["./node_modules/@hono/node-server"],
|
"@hono/node-server": ["../../node_modules/@hono/node-server"],
|
||||||
"atomically": ["./node_modules/atomically"],
|
"atomically": ["../../node_modules/atomically"],
|
||||||
"chokidar": ["./node_modules/chokidar"],
|
"chokidar": ["../../node_modules/chokidar"],
|
||||||
"express": ["./node_modules/@types/express"],
|
"express": ["./node_modules/@types/express"],
|
||||||
"yaml": ["./node_modules/yaml"],
|
"yaml": ["./node_modules/yaml"],
|
||||||
"@napi-rs/keyring": ["./node_modules/@napi-rs/keyring"],
|
"@napi-rs/keyring": ["../../node_modules/@napi-rs/keyring"],
|
||||||
"react-syntax-highlighter/dist/esm/prism-light": [
|
"react-syntax-highlighter/dist/esm/prism-light": [
|
||||||
"./src/types/react-syntax-highlighter-prism-light"
|
"./src/types/react-syntax-highlighter-prism-light"
|
||||||
],
|
],
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue