Merge pull request #1983 from modelcontextprotocol/v2/fix/1952-tui-form-crash
fix(tui): bundle React-rendering deps so a consumer install can't split React
This commit is contained in:
Cliff Hall
GitHub
commit
10793d79fa
8 changed files with 472 additions and 139 deletions
1
.github/copilot-instructions.md
vendored
1
.github/copilot-instructions.md
vendored
|
|
@ -77,6 +77,7 @@ 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 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.
|
||||
- **A dependency that renders React components must be bundled into the client that uses it, and is then not a root dependency.** An externalized package resolves its own `react` from wherever npm placed it in the *consumer's* tree, beside a React satisfying *its* peer range — looser than ours, which is all it takes to split React. `ink-form`/`ink-scroll-view` declare `">=18"`, so a consumer's React 18 satisfies them, the TUI ends up with two React instances, and it crashes on the first hook (#1952). Both are inlined via `noExternal` in `clients/tui/tsup.config.ts`. **`ink` is the one exemption, justified by cost (~1.4MB) — never by a peer range**: flag any claim that `">=19"` keeps npm from misplacing it, which is false and was in this repo once. What keeps it safe is the **root `react` range staying open to the whole major (`^19.0.0`)** so npm can dedupe with a consumer's pinned React 19; treat narrowing that range as reopening the bug. `clients/tui/__tests__/tsupConfig.test.ts` enforces the split, the root-declaration of exempt packages, and that range.
|
||||
- **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
|
||||
|
|
|
|||
|
|
@ -126,6 +126,10 @@ The same **placement** rule covers anything reached only through **root-owned co
|
|||
- 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.
|
||||
|
||||
**A dependency that renders React components must be bundled into the client that uses it, and is then not a root dependency.** An externalized package resolves its own `react` from wherever npm placed **it** in the consumer's tree, and npm places a package beside a React satisfying *that package's* peer range — looser than ours in every case here, which is all it takes to split React. `ink-form` and `ink-scroll-view` declare `">=18"`, satisfied by a consumer's React 18 while our React 19 nests underneath: the bundle renders through one React, those packages call hooks on another, and the TUI crashes on the first hook (#1952). Both are inlined by `clients/tui/tsup.config.ts` (`noExternal`) and declared only in `clients/tui/package.json`, where the build resolves them — declaring an inlined package at the root would just make consumers install a second, unused copy.
|
||||
|
||||
**`ink` is the single exemption, and it is justified by cost, not by safety.** Bundling it works but adds ~1.4MB (`react-reconciler` + `yoga-layout`, plus a `createRequire` banner, since inlined CJS calls `require` at runtime and esbuild's ESM interop rejects that without a real `require` in scope). **Never justify an exemption by a peer range** — `ink` briefly carried "its `">=19"` peer keeps npm honest", which is false: a consumer pinning React 19.0 satisfies `">=19"` while a narrower range of ours nests underneath. What actually makes the exemption safe is a *different* lever: the **root `react` range stays open to the whole major (`^19.0.0`)**, so npm can dedupe our React with whatever React 19 a consumer pins and an external `ink` lands on the same copy the bundle uses. Narrowing it (e.g. back to `^19.2.4`) silently reopens the crash for the renderer itself, which breaks TUI *startup*, not just its forms. `clients/tui/__tests__/tsupConfig.test.ts` enforces all of it: React-rendering deps inlined, each exempt package both external and root-declared, and the root range pinned to `ink`'s peer floor.
|
||||
|
||||
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`.
|
||||
|
||||
## Contributing
|
||||
|
|
|
|||
|
|
@ -310,6 +310,7 @@ The root `package.json` `"files"` allowlist is the source of truth for the tarba
|
|||
- **No source maps.** The client bundlers set `sourcemap: false` (`clients/{cli,tui}/tsup.config.ts`, `clients/web/tsup.runner.config.ts`); Vite and the launcher's `tsc` already emit none. Maps are ~half the unpacked size and aren't needed at runtime — debug via `npm run dev` on the source.
|
||||
- **`clients/web/build` ships via `clients/web/.npmignore`.** `clients/web/.gitignore` lists `build/`, and npm's packlist honors that nested `.gitignore` over the root `"files"` allowlist — so the prod web-server runner was silently missing from the tarball while `clients/web/dist` slipped through (its `.gitignore` only lists `dist-ssr`). `clients/web/.npmignore` overrides the `.gitignore` for publishing so both `build/` (runner) and `dist/` (SPA) ship. The other clients don't need this — none ship a nested `.gitignore`.
|
||||
- **`clients/web/static` ships the MCP Apps sandbox proxy.** `clients/web/static/sandbox_proxy.html` is a committed source file (not a build artifact), read from disk at runtime by `clients/web/server/sandbox-controller.ts` as `<runner dir>/../static/sandbox_proxy.html`. It was missing from the root `"files"` allowlist entirely, so every published build failed the Apps tab with **"Sandbox not loaded"** ([#1859](https://github.com/modelcontextprotocol/inspector/issues/1859)) while working fine in the repo. Because the path is resolved _relative to_ `clients/web/build`, the directory must ship at that exact location — `pack:verify` asserts both the tarball entry and the installed-on-disk path.
|
||||
- **A dependency that renders React is bundled, not externalized.** An externalized package resolves its own `react` from wherever npm placed **it** in the consumer's tree, which is not necessarily where the bundle resolves ours — npm places a package beside a React satisfying *its* peer range, and those ranges are looser than ours. `ink-form` and `ink-scroll-view` declare `">=18"`, so a project holding React 18 satisfies them and gets them hoisted while the Inspector's React 19 nests underneath: two React copies, and the TUI dies with `TypeError: Cannot read properties of null (reading 'useState')` the moment a tool test form or a scroll view mounts ([#1952](https://github.com/modelcontextprotocol/inspector/issues/1952)). Both are therefore inlined by `clients/tui/tsup.config.ts` and are **not** root dependencies: the tarball ships their code inside `clients/tui/build/index.js` rather than having consumers install them. Bundling also pins their transitive deps to what this repo's install resolved (notably `ink-select-input@6` via `overrides`, which npm ignores for a package installed as a dependency). **`ink` is the one exception, on cost:** bundling it works but adds ~1.4 MB (`react-reconciler` and `yoga-layout` come along, plus a `createRequire` banner for the inlined CJS), so it stays external — *not* because its `">=19"` peer makes it safe, which it does not. What keeps that tolerable is the root `react` range: `"^19.0.0"` is deliberately open to the whole major so npm can dedupe our React with whatever React 19 a consumer pins, leaving an external `ink` on the same copy the bundle uses. **Narrowing that range reopens the bug for the renderer itself** — `clients/tui/__tests__/tsupConfig.test.ts` pins it to `ink`'s peer floor, and guards the rest of the split; see the [TUI README](./clients/tui/README.md#bundling-react-rendering-dependencies-must-be-inlined-1952).
|
||||
- **A single version number, read from the root `package.json`.** The Inspector ships as one package with one version, so only the **root** `package.json` carries a `version` — the four `clients/*/package.json`s deliberately have none. Every Node client (CLI, TUI, and the web backend) resolves the version through the shared `readInspectorVersion()` reader in `core/node/version.ts`, which walks up to the root manifest (always present in the tarball). No client `package.json` is read at runtime, so none needs to ship. The web **browser** can't read the filesystem; it gets its version from the backend via `GET /api/config` (see [#1639](https://github.com/modelcontextprotocol/inspector/issues/1639)).
|
||||
|
||||
### `npm run pack:verify` — publish smoke against the real tarball
|
||||
|
|
|
|||
|
|
@ -102,7 +102,93 @@ The repo-root `validate:tui` just delegates here. `eslint.config.js` registers
|
|||
stricter react-hooks@7 rules are not enforced on the interim component surface
|
||||
(#1501).
|
||||
|
||||
Tests live in `__tests__/`. The coverage gate currently covers the TUI's
|
||||
non-React logic (server resolution, logger, tab metadata, and the `utils/`
|
||||
form/URL helpers); the Ink components and `App.tsx` are an interim exclusion in
|
||||
`vitest.config.ts` pending a renderer-based follow-up.
|
||||
Tests live in `__tests__/`. The coverage gate covers **all of `src/**`**, React
|
||||
surface included — the Ink components mount through `ink-testing-library` (with
|
||||
the `ink-scroll-view` / `ink-form` passthrough doubles in `__tests__/helpers/`),
|
||||
`App.tsx` mounts against a mock of the `@inspector/core` surface, and keypresses
|
||||
are driven through stdin. The former interim exclusion of the components and
|
||||
`App.tsx` was lifted in #1501; the only exclusion left in `vitest.config.ts` is
|
||||
`src/tui-servers.ts`, a pure re-export of core's server resolver with no runtime
|
||||
statements of its own (its logic is measured in `core/` via the web suite, and
|
||||
`tui-servers.test.ts` still exercises it behaviorally — it is excluded only so it
|
||||
doesn't surface as a misleading 0/0 row).
|
||||
|
||||
### Bundling: React-rendering dependencies must be inlined (#1952)
|
||||
|
||||
`tsup.config.ts` splits the TUI's dependencies into bundled (`noExternal`) and
|
||||
externalized (`external`). For anything that **renders React components**, that
|
||||
choice decides which React instance it gets at runtime, and getting it wrong
|
||||
crashes the TUI in a way nothing in this repo can see.
|
||||
|
||||
An external package's `import "react"` resolves from wherever npm placed **that
|
||||
package** — and npm places it beside a React satisfying *its* peer range, which
|
||||
is looser than ours in every case here:
|
||||
|
||||
| Package | Its `react` peer | Placed beside a different React when… |
|
||||
| --- | --- | --- |
|
||||
| `ink-form`, `ink-scroll-view` | `">=18"` | the consumer has React 18 |
|
||||
| `ink` | `">=19"` | the consumer pins React 19.0 (our `^19.2.4` then nests) |
|
||||
|
||||
Either way the bundle renders through one React while the external package calls
|
||||
hooks on another, whose dispatcher is null — so opening a tool test form (or any
|
||||
scroll view, or in `ink`'s case simply starting the TUI) dies with
|
||||
`TypeError: Cannot read properties of null (reading 'useState')`.
|
||||
|
||||
`ink-form` and `ink-scroll-view` are therefore **inlined**, which removes npm
|
||||
from the decision: an inlined package's `import "react"` is emitted into
|
||||
`build/index.js`, so it resolves from the build directory exactly like the
|
||||
bundle's own. Inlining also pins transitive deps to what *this* install
|
||||
resolved, notably `ink-select-input@6` via the `overrides` entry — npm ignores a
|
||||
dependency's `overrides`, so a consumer install would otherwise pull the
|
||||
React-18-era v5 that `ink-form` asks for.
|
||||
|
||||
#### Why `ink` itself is the exception
|
||||
|
||||
`ink` is external for **cost**, not because its `">=19"` peer makes it safe — it
|
||||
does not, and an earlier revision of this section wrongly claimed it did.
|
||||
Bundling `ink` works (it was built and verified against both consumer repros)
|
||||
but adds ~1.4 MB, since `react-reconciler` and `yoga-layout` come with it, plus
|
||||
a `createRequire` banner: inlined CJS calls `require` at runtime
|
||||
(`react-reconciler` for `"react"`, `signal-exit@3` for `"assert"`) and esbuild's
|
||||
interop shim throws `Dynamic require of "x" is not supported` without a real
|
||||
`require` in scope. A 602 KB bundle beat a 2 MB one.
|
||||
|
||||
What makes the exemption tolerable is a **separate lever**: the root manifest
|
||||
declares `react: "^19.0.0"` — open to the whole major, rather than pinned at the
|
||||
version we happen to develop against. That lets npm satisfy our React and a
|
||||
consumer's pinned React 19.x with a single copy, so an external `ink` resolves
|
||||
*ours*. Verified against a consumer pinning `react@19.0.0` alongside `ink@6.8.0`
|
||||
— the case that splits under a narrower range:
|
||||
|
||||
```
|
||||
bundle react node_modules/react/index.js
|
||||
ink -> react node_modules/react/index.js # same copy
|
||||
```
|
||||
|
||||
Narrow that range and the exemption turns straight back into #1952, one level
|
||||
up — breaking TUI startup rather than just its forms. `tsupConfig.test.ts` pins
|
||||
the root range to `ink`'s own peer floor so it can't drift silently. The
|
||||
residual after all this is a React **20**-era consumer that also depends on
|
||||
`ink`; that gets revisited when the Inspector moves to React 20.
|
||||
|
||||
`__tests__/tsupConfig.test.ts` enforces the whole split: every dependency
|
||||
declaring a `react` peer must be in `noExternal` unless it is listed as external
|
||||
by design, each exempt package must also be a root dependency (external means
|
||||
consumers install it), and the root `react` range must stay open to the major.
|
||||
Add a React-rendering dependency, and that test tells you to bundle it.
|
||||
|
||||
### The `ink-form` label patch
|
||||
|
||||
Bundling `ink-form` also makes it patchable, which one label needs: the hint
|
||||
under an incomplete form reads "you have not **competed** yet". It is upstream's
|
||||
string, hardcoded in `ink-form/lib/SubmitButton.js` with no prop to override,
|
||||
and `ink-form` was last published in 2024 — so `tsup.config.ts` corrects it with
|
||||
an esbuild `onLoad` hook as the file enters the bundle. It is reported upstream
|
||||
as [lukasbach/ink-form#14](https://github.com/lukasbach/ink-form/issues/14); if
|
||||
a release ever carries the fix, drop the patch.
|
||||
|
||||
The hook **throws** when the string isn't found rather than passing the source
|
||||
through. A silent no-op would let an `ink-form` upgrade retire the patch without
|
||||
anyone noticing it had stopped applying — or leave a patch aimed at a string
|
||||
that no longer exists. If the build fails there, check whether upstream fixed
|
||||
the typo and delete the patch instead of re-targeting it.
|
||||
|
|
|
|||
262
clients/tui/__tests__/tsupConfig.test.ts
Normal file
262
clients/tui/__tests__/tsupConfig.test.ts
Normal file
|
|
@ -0,0 +1,262 @@
|
|||
/**
|
||||
* Bundling invariant for the TUI's React-rendering dependencies (#1952).
|
||||
*
|
||||
* The published TUI is a single ESM bundle whose bare `import "react"` resolves
|
||||
* from `clients/tui/build/`. Any package left *external* resolves its own
|
||||
* `react` from wherever npm placed **it** instead — and npm places a package
|
||||
* next to a version satisfying its declared peer range. `ink-form` and
|
||||
* `ink-scroll-view` accept `react: ">=18"`, so a consumer project holding React
|
||||
* 18 satisfies them, they hoist to that project's root, and the Inspector's
|
||||
* React 19 nests beneath it. Two React copies later, the first hook either of
|
||||
* them calls reads a null dispatcher and the TUI dies with
|
||||
* "Cannot read properties of null (reading 'useState')" — the moment a tool
|
||||
* test form or a scroll view mounts.
|
||||
*
|
||||
* Inlining them removes npm from the decision entirely: their `import "react"`
|
||||
* is emitted into the bundle, so it resolves exactly where the bundle's does.
|
||||
*
|
||||
* This test is the durable guard, because the failure is invisible in the repo
|
||||
* (a dev install has one React) and in every smoke (same) — it only appears
|
||||
* once the package is installed *under* another project that renders React.
|
||||
*/
|
||||
|
||||
import { describe, it, expect } from "vitest";
|
||||
import { readFileSync } from "node:fs";
|
||||
import path from "node:path";
|
||||
import { fileURLToPath } from "node:url";
|
||||
import type { Options } from "tsup";
|
||||
import tsupConfig, {
|
||||
INK_FORM_INCOMPLETE_HINT,
|
||||
INK_FORM_SUBMIT_BUTTON,
|
||||
fixInkFormIncompleteHint,
|
||||
inkFormLabelPatch,
|
||||
} from "../tsup.config.js";
|
||||
|
||||
const clientDir = path.resolve(
|
||||
path.dirname(fileURLToPath(import.meta.url)),
|
||||
"..",
|
||||
);
|
||||
|
||||
/**
|
||||
* `defineConfig()` is typed as a union — a single options object, an array of
|
||||
* them, or a function returning either. The TUI config is the first form, so
|
||||
* narrow to it at runtime rather than assuming: a future config that grows a
|
||||
* second build should fail loudly here instead of silently checking nothing.
|
||||
*/
|
||||
function singleConfig(config: typeof tsupConfig): Options {
|
||||
if (typeof config !== "object" || Array.isArray(config)) {
|
||||
throw new Error(
|
||||
"clients/tui/tsup.config.ts is expected to export one options object",
|
||||
);
|
||||
}
|
||||
return config;
|
||||
}
|
||||
|
||||
/** The string entries of an `external` / `noExternal` list (ours are all strings but the type allows RegExp). */
|
||||
function stringEntries(list: (string | RegExp)[] | undefined): string[] {
|
||||
return (list ?? []).filter((entry) => typeof entry === "string");
|
||||
}
|
||||
|
||||
/**
|
||||
* React-rendering dependencies left external by an explicit trade-off.
|
||||
*
|
||||
* `ink` is here for its **cost**, not because its peer range makes it safe.
|
||||
* Bundling it works (verified against both consumer repros) but adds ~1.4MB —
|
||||
* react-reconciler and yoga-layout come with it — plus a `createRequire` banner
|
||||
* for the inlined CJS. The smaller tarball won.
|
||||
*
|
||||
* The exemption is only tolerable because the root manifest keeps `react` open
|
||||
* to the whole major (`^19.0.0`), which lets npm dedupe our React with whatever
|
||||
* React 19 a consumer pins — so an external `ink` resolves the same copy the
|
||||
* bundle does. The test below pins that relationship: narrowing the root range
|
||||
* silently reopens #1952 for the renderer itself.
|
||||
*/
|
||||
const EXTERNAL_BY_DESIGN = new Set(["ink"]);
|
||||
|
||||
function readPackageJson(name: string): {
|
||||
peerDependencies?: Record<string, string>;
|
||||
} {
|
||||
// Resolve through the client's own node_modules rather than `require.resolve`
|
||||
// on the package name: several of these packages export only their entry
|
||||
// point, so `<name>/package.json` is not a resolvable subpath.
|
||||
const manifest = path.join(clientDir, "node_modules", name, "package.json");
|
||||
return JSON.parse(readFileSync(manifest, "utf8"));
|
||||
}
|
||||
|
||||
/** Dependencies that declare a `react` peer — i.e. that render React themselves. */
|
||||
function reactRenderingDependencies(): string[] {
|
||||
const { dependencies } = JSON.parse(
|
||||
readFileSync(path.join(clientDir, "package.json"), "utf8"),
|
||||
) as { dependencies: Record<string, string> };
|
||||
|
||||
return Object.keys(dependencies).filter(
|
||||
(name) => readPackageJson(name).peerDependencies?.react !== undefined,
|
||||
);
|
||||
}
|
||||
|
||||
/** The real `ink-form` module the label patch targets. */
|
||||
const submitButtonPath = path.join(
|
||||
clientDir,
|
||||
"node_modules",
|
||||
"ink-form",
|
||||
"lib",
|
||||
"SubmitButton.js",
|
||||
);
|
||||
|
||||
const config = singleConfig(tsupConfig);
|
||||
const noExternal = stringEntries(config.noExternal);
|
||||
const external = stringEntries(config.external);
|
||||
|
||||
describe("tui tsup config", () => {
|
||||
it("finds the React-rendering dependencies to check", () => {
|
||||
// A guard on the guard: if this resolves to nothing (a rename, a moved
|
||||
// node_modules), every assertion below would pass vacuously.
|
||||
expect(reactRenderingDependencies().length).toBeGreaterThan(0);
|
||||
});
|
||||
|
||||
it("bundles every React-rendering dependency that is not exempt", () => {
|
||||
const shouldInline = reactRenderingDependencies().filter(
|
||||
(name) => !EXTERNAL_BY_DESIGN.has(name),
|
||||
);
|
||||
|
||||
expect(shouldInline.length).toBeGreaterThan(0);
|
||||
for (const name of shouldInline) {
|
||||
expect(
|
||||
noExternal,
|
||||
`${name} renders React, so it must be bundled`,
|
||||
).toContain(name);
|
||||
expect(external, `${name} must not be external`).not.toContain(name);
|
||||
}
|
||||
});
|
||||
|
||||
it("keeps the root react range open to the whole major, so npm can dedupe", () => {
|
||||
// This is what keeps the one exemption tolerable. `ink` is external, so it
|
||||
// resolves whatever React npm placed beside it — and npm can only place it
|
||||
// beside *ours* if our range admits the consumer's React too. Pinning the
|
||||
// root range above the major floor (say `^19.2.4`) means a consumer holding
|
||||
// React 19.0 gets a second copy: `ink` renders through theirs, the bundle
|
||||
// through ours, and the TUI dies at startup on a null dispatcher.
|
||||
//
|
||||
// So the root range must start at the same floor `ink`'s own peer does.
|
||||
const rootReact = (
|
||||
JSON.parse(
|
||||
readFileSync(path.join(clientDir, "..", "..", "package.json"), "utf8"),
|
||||
) as { dependencies: Record<string, string> }
|
||||
).dependencies.react;
|
||||
const inkPeerReact = readPackageJson("ink").peerDependencies?.react ?? "";
|
||||
|
||||
const floor = /^>=(\d+)\.0\.0$/.exec(inkPeerReact)?.[1];
|
||||
expect(floor, `unexpected ink peer range: ${inkPeerReact}`).toBeDefined();
|
||||
expect(
|
||||
rootReact,
|
||||
`root react must be ^${floor}.0.0 so npm can dedupe with any React ${floor} a consumer pins`,
|
||||
).toBe(`^${floor}.0.0`);
|
||||
});
|
||||
|
||||
it("keeps each exempt package external, and declared for consumers", () => {
|
||||
// An external package is not shipped in the bundle, so the root manifest
|
||||
// has to install it — the mirror image of the inlined ones, which must NOT
|
||||
// be root dependencies. Getting this backwards breaks the published TUI at
|
||||
// startup with an unresolved import.
|
||||
const rootDependencies = JSON.parse(
|
||||
readFileSync(path.join(clientDir, "..", "..", "package.json"), "utf8"),
|
||||
) as { dependencies: Record<string, string> };
|
||||
|
||||
for (const name of EXTERNAL_BY_DESIGN) {
|
||||
expect(external, `${name} is external by design`).toContain(name);
|
||||
expect(
|
||||
Object.keys(rootDependencies.dependencies),
|
||||
`${name} is external, so consumers must install it`,
|
||||
).toContain(name);
|
||||
}
|
||||
});
|
||||
|
||||
it("keeps react itself external, as the single shared instance", () => {
|
||||
expect(external).toContain("react");
|
||||
expect(noExternal).not.toContain("react");
|
||||
});
|
||||
|
||||
it("corrects ink-form's misspelled incomplete-form hint", () => {
|
||||
// Read the real dependency, so an `ink-form` upgrade that fixes or rewords
|
||||
// the label fails here — the patch must then be removed, not left silently
|
||||
// matching nothing.
|
||||
const patched = fixInkFormIncompleteHint(
|
||||
readFileSync(submitButtonPath, "utf8"),
|
||||
submitButtonPath,
|
||||
);
|
||||
expect(patched).toContain(INK_FORM_INCOMPLETE_HINT.fixed);
|
||||
expect(patched).not.toContain(INK_FORM_INCOMPLETE_HINT.typo);
|
||||
});
|
||||
|
||||
it("routes the real module through the plugin, not just the helper", async () => {
|
||||
// The helper is only reached if the plugin's onLoad filter matches. Drive
|
||||
// the plugin as esbuild would — register, then invoke — so a filter that
|
||||
// stops matching the real module's path fails here instead of no-opping
|
||||
// through a green build (the exact silence this patch exists to prevent).
|
||||
type OnLoadCallback = (args: {
|
||||
path: string;
|
||||
}) => Promise<{ contents: string }>;
|
||||
const registered: { filter: RegExp; callback: OnLoadCallback }[] = [];
|
||||
|
||||
const build = {
|
||||
onLoad: (options: { filter: RegExp }, callback: OnLoadCallback) =>
|
||||
registered.push({ filter: options.filter, callback }),
|
||||
};
|
||||
// esbuild's `PluginBuild` carries far more than this patch touches, and the
|
||||
// stub above deliberately implements only the one hook it registers — so
|
||||
// the double cast is bridging a real structural gap, not hiding a mismatch.
|
||||
// A hook the patch called but the stub lacks fails as undefined here rather
|
||||
// than passing silently.
|
||||
inkFormLabelPatch.setup(
|
||||
build as unknown as Parameters<typeof inkFormLabelPatch.setup>[0],
|
||||
);
|
||||
|
||||
expect(registered).toHaveLength(1);
|
||||
const [{ filter, callback }] = registered;
|
||||
expect(filter.test(submitButtonPath)).toBe(true);
|
||||
|
||||
const { contents } = await callback({ path: submitButtonPath });
|
||||
expect(contents).toContain(INK_FORM_INCOMPLETE_HINT.fixed);
|
||||
expect(contents).not.toContain(INK_FORM_INCOMPLETE_HINT.typo);
|
||||
});
|
||||
|
||||
it("scopes the patch to ink-form's SubmitButton and nothing else", () => {
|
||||
expect(INK_FORM_SUBMIT_BUTTON.test(submitButtonPath)).toBe(true);
|
||||
// A Windows-style path must match too — the filter runs against whatever
|
||||
// esbuild resolved, and its separator is the platform's.
|
||||
expect(
|
||||
INK_FORM_SUBMIT_BUTTON.test(
|
||||
"C:\\repo\\node_modules\\ink-form\\lib\\SubmitButton.js",
|
||||
),
|
||||
).toBe(true);
|
||||
for (const other of [
|
||||
"/repo/node_modules/ink-form/lib/Form.js",
|
||||
"/repo/node_modules/ink-select-input/build/SubmitButton.js",
|
||||
"/repo/src/SubmitButton.jsx",
|
||||
]) {
|
||||
expect(INK_FORM_SUBMIT_BUTTON.test(other), other).toBe(false);
|
||||
}
|
||||
});
|
||||
|
||||
it("fails loudly rather than silently skipping a label it cannot find", () => {
|
||||
expect(() =>
|
||||
fixInkFormIncompleteHint("no such label here", "SubmitButton.js"),
|
||||
).toThrow(/no longer contains the ink-form label/);
|
||||
});
|
||||
|
||||
it("only rewrites the misspelling", () => {
|
||||
// The two strings must differ by exactly the fix, or the patch is silently
|
||||
// changing copy nobody reviewed.
|
||||
expect(INK_FORM_INCOMPLETE_HINT.fixed).toBe(
|
||||
INK_FORM_INCOMPLETE_HINT.typo.replace("competed", "completed"),
|
||||
);
|
||||
});
|
||||
|
||||
it("verifies the deps it exempts are still declared", () => {
|
||||
// If `ink` ever leaves the dependency list, the exemption above is stale
|
||||
// and would silently excuse a future package that took its name.
|
||||
for (const name of EXTERNAL_BY_DESIGN) {
|
||||
expect(reactRenderingDependencies()).toContain(name);
|
||||
}
|
||||
});
|
||||
});
|
||||
|
|
@ -1,10 +1,76 @@
|
|||
import { defineConfig } from "tsup";
|
||||
import { defineConfig, type Options } from "tsup";
|
||||
import { readFile } from "node:fs/promises";
|
||||
import path from "node:path";
|
||||
import { fileURLToPath } from "node:url";
|
||||
|
||||
const dirname = path.dirname(fileURLToPath(import.meta.url));
|
||||
const repoRoot = path.resolve(dirname, "../..");
|
||||
|
||||
/**
|
||||
* `ink-form` hardcodes a misspelled hint under an incomplete form — "you have
|
||||
* not competed yet" — with no prop to override it. It is upstream's string
|
||||
* (`ink-form/lib/SubmitButton.js`), last published in 2024, but it renders in
|
||||
* the Inspector's tool/prompt/resource test forms, so we correct it on the way
|
||||
* into the bundle. Reported upstream as lukasbach/ink-form#14; drop this patch
|
||||
* if a release ever carries the fix.
|
||||
*
|
||||
* This is only possible because `ink-form` is inlined (see `noExternal` below).
|
||||
*/
|
||||
export const INK_FORM_INCOMPLETE_HINT = {
|
||||
typo: "There are still required inputs you have not competed yet.",
|
||||
fixed: "There are still required inputs you have not completed yet.",
|
||||
};
|
||||
|
||||
/**
|
||||
* Applies the correction above, throwing if the string is no longer there.
|
||||
*
|
||||
* Failing loudly is the point: a silent no-op would let an `ink-form` upgrade
|
||||
* (or a fixed upstream, or a reworded label) quietly retire this patch with
|
||||
* nobody noticing it had stopped applying — or, worse, leave a patch here for a
|
||||
* string that no longer exists. If this throws, check whether upstream fixed
|
||||
* the typo; if so, delete this patch rather than re-targeting it.
|
||||
*/
|
||||
export function fixInkFormIncompleteHint(source: string, file: string): string {
|
||||
if (!source.includes(INK_FORM_INCOMPLETE_HINT.typo)) {
|
||||
throw new Error(
|
||||
`${file} no longer contains the ink-form label this build patches ` +
|
||||
`(${JSON.stringify(INK_FORM_INCOMPLETE_HINT.typo)}). If upstream fixed ` +
|
||||
`the typo, remove fixInkFormIncompleteHint from tsup.config.ts.`,
|
||||
);
|
||||
}
|
||||
return source.replaceAll(
|
||||
INK_FORM_INCOMPLETE_HINT.typo,
|
||||
INK_FORM_INCOMPLETE_HINT.fixed,
|
||||
);
|
||||
}
|
||||
|
||||
/**
|
||||
* Which module the patch above is applied to.
|
||||
*
|
||||
* Exported so the tests can check it against the *resolved* path of the real
|
||||
* `ink-form` module — a filter that stops matching is the one way this patch
|
||||
* can silently no-op, since `fixInkFormIncompleteHint` would then never run.
|
||||
*/
|
||||
export const INK_FORM_SUBMIT_BUTTON = /ink-form[\\/]lib[\\/]SubmitButton\.js$/;
|
||||
|
||||
// The plugin type is derived from tsup rather than imported from `esbuild`:
|
||||
// `esbuild` is tsup's transitive dependency, not a declared one of this client,
|
||||
// so a direct import typechecks only while npm happens to hoist it.
|
||||
type EsbuildPlugin = NonNullable<Options["esbuildPlugins"]>[number];
|
||||
|
||||
export const inkFormLabelPatch: EsbuildPlugin = {
|
||||
name: "ink-form-label-patch",
|
||||
setup(build) {
|
||||
build.onLoad(
|
||||
{ filter: INK_FORM_SUBMIT_BUTTON },
|
||||
async ({ path: file }) => ({
|
||||
contents: fixInkFormIncompleteHint(await readFile(file, "utf8"), file),
|
||||
loader: "js",
|
||||
}),
|
||||
);
|
||||
},
|
||||
};
|
||||
|
||||
export default defineConfig({
|
||||
entry: ["index.ts"],
|
||||
format: ["esm"],
|
||||
|
|
@ -15,12 +81,39 @@ export default defineConfig({
|
|||
sourcemap: false,
|
||||
target: "node22",
|
||||
platform: "node",
|
||||
noExternal: [/^@inspector\/core/],
|
||||
// Every package here renders React components, so it MUST share the one React
|
||||
// instance the bundle imports. Bundling is what guarantees that: an inlined
|
||||
// package's `import "react"` is emitted into build/index.js, so it resolves
|
||||
// from *this* directory exactly like the bundle's own, and no consumer install
|
||||
// layout can point it elsewhere (#1952).
|
||||
//
|
||||
// Left external, npm is free to place a package beside a *different* React,
|
||||
// because it places one beside a version satisfying that package's own peer
|
||||
// range — looser than ours in every case here. `ink-form` and
|
||||
// `ink-scroll-view` accept ">=18", so a consumer's React 18 satisfies them
|
||||
// while the Inspector's React 19 nests underneath: two React copies, and the
|
||||
// first hook they call reads a null dispatcher ("Cannot read properties of
|
||||
// null (reading 'useState')") the moment a tool test form or a scroll view
|
||||
// mounts. That is the reported crash, and inlining them is its fix.
|
||||
//
|
||||
// `__tests__/tsupConfig.test.ts` guards this list.
|
||||
noExternal: [/^@inspector\/core/, "ink-form", "ink-scroll-view"],
|
||||
external: [
|
||||
// `react` is deliberately external — the single instance every inlined
|
||||
// package above resolves to, from this build directory.
|
||||
"react",
|
||||
// `ink` is external by a deliberate trade-off, NOT because a ">=19" peer
|
||||
// makes it safe — it does not, and that claim was wrong here once already.
|
||||
// Bundling it works (verified) but costs ~1.4MB, since react-reconciler and
|
||||
// yoga-layout come with it, plus a `createRequire` banner for the inlined
|
||||
// CJS. The smaller tarball won.
|
||||
//
|
||||
// What makes that tolerable is the root manifest's `react: ^19.0.0`: being
|
||||
// open to the whole major lets npm satisfy our React and a consumer's
|
||||
// pinned one with a single copy, so an external `ink` resolves *ours*. Narrow
|
||||
// that range and this exemption turns back into the #1952 crash, one level
|
||||
// up — `__tests__/tsupConfig.test.ts` guards it.
|
||||
"ink",
|
||||
"ink-form",
|
||||
"ink-scroll-view",
|
||||
"open",
|
||||
"commander",
|
||||
"pino",
|
||||
|
|
@ -28,6 +121,7 @@ export default defineConfig({
|
|||
"@modelcontextprotocol/core",
|
||||
"@napi-rs/keyring",
|
||||
],
|
||||
esbuildPlugins: [inkFormLabelPatch],
|
||||
esbuildOptions(options) {
|
||||
options.alias = {
|
||||
"@inspector/core": path.join(repoRoot, "core"),
|
||||
|
|
|
|||
140
package-lock.json
generated
140
package-lock.json
generated
|
|
@ -24,11 +24,9 @@
|
|||
"commander": "^13.1.0",
|
||||
"hono": "^4.12.18",
|
||||
"ink": "^6.0.0",
|
||||
"ink-form": "^2.0.1",
|
||||
"ink-scroll-view": "^0.3.6",
|
||||
"open": "^10.2.0",
|
||||
"pino": "^9.14.0",
|
||||
"react": "^19.2.4",
|
||||
"react": "^19.0.0",
|
||||
"undici": "^8.5.0",
|
||||
"vite": "^8.1.5",
|
||||
"yaml": "^2.9.0",
|
||||
|
|
@ -1821,13 +1819,14 @@
|
|||
}
|
||||
},
|
||||
"node_modules/es-toolkit": {
|
||||
"version": "1.47.0",
|
||||
"resolved": "https://registry.npmjs.org/es-toolkit/-/es-toolkit-1.47.0.tgz",
|
||||
"integrity": "sha512-n1GuoD0WEQZMBk5tttoZSqwgyLx01oqa5XsBmCHwPyNe1S9jPBEmtR2pSgp2kJuWE3ciFZ6yRHmY4pM4C3OOkw==",
|
||||
"version": "1.50.0",
|
||||
"resolved": "https://registry.npmjs.org/es-toolkit/-/es-toolkit-1.50.0.tgz",
|
||||
"integrity": "sha512-OyZKhUVvEep9ITEiwHn8GKnMRQIVqoSIX7WnRbkWgJkllCujilqP2rD0u979tkl8wqyc8ICwlc1UBVv/Sl1G6w==",
|
||||
"license": "MIT",
|
||||
"workspaces": [
|
||||
"docs",
|
||||
"benchmarks"
|
||||
"benchmarks",
|
||||
"tests/types"
|
||||
]
|
||||
},
|
||||
"node_modules/escape-html": {
|
||||
|
|
@ -2181,21 +2180,6 @@
|
|||
}
|
||||
}
|
||||
},
|
||||
"node_modules/figures": {
|
||||
"version": "6.1.0",
|
||||
"resolved": "https://registry.npmjs.org/figures/-/figures-6.1.0.tgz",
|
||||
"integrity": "sha512-d+l3qxjSesT4V7v2fh+QnmFnUWv9lSpjarhShNTgBOfA0ttejbQUAlHLitbjkoRiDulW0OPoQPYIGhIC8ohejg==",
|
||||
"license": "MIT",
|
||||
"dependencies": {
|
||||
"is-unicode-supported": "^2.0.0"
|
||||
},
|
||||
"engines": {
|
||||
"node": ">=18"
|
||||
},
|
||||
"funding": {
|
||||
"url": "https://github.com/sponsors/sindresorhus"
|
||||
}
|
||||
},
|
||||
"node_modules/file-entry-cache": {
|
||||
"version": "8.0.0",
|
||||
"resolved": "https://registry.npmjs.org/file-entry-cache/-/file-entry-cache-8.0.0.tgz",
|
||||
|
|
@ -2552,76 +2536,6 @@
|
|||
}
|
||||
}
|
||||
},
|
||||
"node_modules/ink-form": {
|
||||
"version": "2.0.1",
|
||||
"resolved": "https://registry.npmjs.org/ink-form/-/ink-form-2.0.1.tgz",
|
||||
"integrity": "sha512-vo0VMwHf+HOOJo7026K4vJEN8xm4sP9iWlQLx4bngNEEY5K8t30CUvVjQCCNAV6Mt2ODt2Aq+2crCuBONReJUg==",
|
||||
"license": "MIT",
|
||||
"dependencies": {
|
||||
"ink-select-input": "^5.0.0",
|
||||
"ink-text-input": "^6.0.0"
|
||||
},
|
||||
"peerDependencies": {
|
||||
"ink": ">=4",
|
||||
"react": ">=18"
|
||||
}
|
||||
},
|
||||
"node_modules/ink-scroll-view": {
|
||||
"version": "0.3.7",
|
||||
"resolved": "https://registry.npmjs.org/ink-scroll-view/-/ink-scroll-view-0.3.7.tgz",
|
||||
"integrity": "sha512-lUBLxSbVry/+UtJhuvRu6wumP43+ScVp69J7e+hmYwz1kTkahWfkVwWOu7Mn1DMPb8AU8bnsmEsr6kvSJvnaRw==",
|
||||
"license": "MIT",
|
||||
"peerDependencies": {
|
||||
"ink": "^5 || ^6 || ^7",
|
||||
"react": "^18 || ^19"
|
||||
}
|
||||
},
|
||||
"node_modules/ink-select-input": {
|
||||
"version": "6.2.0",
|
||||
"resolved": "https://registry.npmjs.org/ink-select-input/-/ink-select-input-6.2.0.tgz",
|
||||
"integrity": "sha512-304fZXxkpYxJ9si5lxRCaX01GNlmPBgOZumXXRnPYbHW/iI31cgQynqk2tRypGLOF1cMIwPUzL2LSm6q4I5rQQ==",
|
||||
"license": "MIT",
|
||||
"dependencies": {
|
||||
"figures": "^6.1.0",
|
||||
"to-rotated": "^1.0.0"
|
||||
},
|
||||
"engines": {
|
||||
"node": ">=18"
|
||||
},
|
||||
"peerDependencies": {
|
||||
"ink": ">=5.0.0",
|
||||
"react": ">=18.0.0"
|
||||
}
|
||||
},
|
||||
"node_modules/ink-text-input": {
|
||||
"version": "6.0.0",
|
||||
"resolved": "https://registry.npmjs.org/ink-text-input/-/ink-text-input-6.0.0.tgz",
|
||||
"integrity": "sha512-Fw64n7Yha5deb1rHY137zHTAbSTNelUKuB5Kkk2HACXEtwIHBCf9OH2tP/LQ9fRYTl1F0dZgbW0zPnZk6FA9Lw==",
|
||||
"license": "MIT",
|
||||
"dependencies": {
|
||||
"chalk": "^5.3.0",
|
||||
"type-fest": "^4.18.2"
|
||||
},
|
||||
"engines": {
|
||||
"node": ">=18"
|
||||
},
|
||||
"peerDependencies": {
|
||||
"ink": ">=5",
|
||||
"react": ">=18"
|
||||
}
|
||||
},
|
||||
"node_modules/ink-text-input/node_modules/type-fest": {
|
||||
"version": "4.41.0",
|
||||
"resolved": "https://registry.npmjs.org/type-fest/-/type-fest-4.41.0.tgz",
|
||||
"integrity": "sha512-TeTSQ6H5YHvpqVwBRcnLDCBnDOHWYu7IvGbHT6N8AOymcr9PJGjc1GTtiWZTYg0NCgYwvnYWEkVChQAr9bjfwA==",
|
||||
"license": "(MIT OR CC0-1.0)",
|
||||
"engines": {
|
||||
"node": ">=16"
|
||||
},
|
||||
"funding": {
|
||||
"url": "https://github.com/sponsors/sindresorhus"
|
||||
}
|
||||
},
|
||||
"node_modules/ip-address": {
|
||||
"version": "10.1.0",
|
||||
"resolved": "https://registry.npmjs.org/ip-address/-/ip-address-10.1.0.tgz",
|
||||
|
|
@ -2732,18 +2646,6 @@
|
|||
"integrity": "sha512-hvpoI6korhJMnej285dSg6nu1+e6uxs7zG3BYAm5byqDsgJNWwxzM6z6iZiAgQR4TJ30JmBTOwqZUw3WlyH3AQ==",
|
||||
"license": "MIT"
|
||||
},
|
||||
"node_modules/is-unicode-supported": {
|
||||
"version": "2.1.0",
|
||||
"resolved": "https://registry.npmjs.org/is-unicode-supported/-/is-unicode-supported-2.1.0.tgz",
|
||||
"integrity": "sha512-mE00Gnza5EEB3Ds0HfMyllZzbBrmLOX3vfWoj9A9PEnTfratQ/BcaJOuMhnkhjXvb2+FkY3VuHqtAGpTPmglFQ==",
|
||||
"license": "MIT",
|
||||
"engines": {
|
||||
"node": ">=18"
|
||||
},
|
||||
"funding": {
|
||||
"url": "https://github.com/sponsors/sindresorhus"
|
||||
}
|
||||
},
|
||||
"node_modules/is-wsl": {
|
||||
"version": "3.1.1",
|
||||
"resolved": "https://registry.npmjs.org/is-wsl/-/is-wsl-3.1.1.tgz",
|
||||
|
|
@ -3986,9 +3888,9 @@
|
|||
}
|
||||
},
|
||||
"node_modules/string-width": {
|
||||
"version": "8.2.1",
|
||||
"resolved": "https://registry.npmjs.org/string-width/-/string-width-8.2.1.tgz",
|
||||
"integrity": "sha512-IIaP0g3iy9Cyy18w3M9YcaDudujEAVHKt3a3QJg1+sr/oX96TbaGUubG0hJyCjCBThFH+tFpcIyoUHUn1ogaLA==",
|
||||
"version": "8.2.2",
|
||||
"resolved": "https://registry.npmjs.org/string-width/-/string-width-8.2.2.tgz",
|
||||
"integrity": "sha512-GaPUh5gfdrYzqeVNZvUfT23vYYxXzKYidUcnMtJg/3rxRV63EFZy3k6xfKlmfeJD0176lnUV/Usr3XcwSvFzpg==",
|
||||
"license": "MIT",
|
||||
"dependencies": {
|
||||
"get-east-asian-width": "^1.5.0",
|
||||
|
|
@ -4080,18 +3982,6 @@
|
|||
"url": "https://github.com/sponsors/SuperchupuDev"
|
||||
}
|
||||
},
|
||||
"node_modules/to-rotated": {
|
||||
"version": "1.0.0",
|
||||
"resolved": "https://registry.npmjs.org/to-rotated/-/to-rotated-1.0.0.tgz",
|
||||
"integrity": "sha512-KsEID8AfgUy+pxVRLsWp0VzCa69wxzUDZnzGbyIST/bcgcrMvTYoFBX/QORH4YApoD89EDuUovx4BTdpOn319Q==",
|
||||
"license": "MIT",
|
||||
"engines": {
|
||||
"node": ">=18"
|
||||
},
|
||||
"funding": {
|
||||
"url": "https://github.com/sponsors/sindresorhus"
|
||||
}
|
||||
},
|
||||
"node_modules/toidentifier": {
|
||||
"version": "1.0.1",
|
||||
"resolved": "https://registry.npmjs.org/toidentifier/-/toidentifier-1.0.1.tgz",
|
||||
|
|
@ -4135,9 +4025,9 @@
|
|||
}
|
||||
},
|
||||
"node_modules/type-fest": {
|
||||
"version": "5.7.0",
|
||||
"resolved": "https://registry.npmjs.org/type-fest/-/type-fest-5.7.0.tgz",
|
||||
"integrity": "sha512-1URUxUqfHFM1c+zfSPsa3gnkO7Aq21qyH75SIduNYz4SzY964rn1X2vCMQaHSHhktiw+0kPa2iyb6PUpXqB6Vg==",
|
||||
"version": "5.8.0",
|
||||
"resolved": "https://registry.npmjs.org/type-fest/-/type-fest-5.8.0.tgz",
|
||||
"integrity": "sha512-YGYEVz3Fm5iy/AybuA0oyNFq7H4CgQNfRp/qfe8nurE1kuCeNm3/vfm9X4Mtl+qLyaKJUh5xrFZwogr41SMjYA==",
|
||||
"license": "(MIT OR CC0-1.0)",
|
||||
"dependencies": {
|
||||
"tagged-tag": "^1.0.0"
|
||||
|
|
@ -4402,9 +4292,9 @@
|
|||
"license": "ISC"
|
||||
},
|
||||
"node_modules/ws": {
|
||||
"version": "8.21.0",
|
||||
"resolved": "https://registry.npmjs.org/ws/-/ws-8.21.0.tgz",
|
||||
"integrity": "sha512-Vsp28b7DRcimFQvrqu2Wek3z1iYxDCWqHYB8Qsnk/S4RfaCQzPGPyBNuVjJV3cd6UiKtUtp6sNM77gWvzcCH+g==",
|
||||
"version": "8.21.3",
|
||||
"resolved": "https://registry.npmjs.org/ws/-/ws-8.21.3.tgz",
|
||||
"integrity": "sha512-201TZ/kPWxoPr/OKWjquZR1SWKXcvxdH+e1xrx89b3YbmzLMFCLfnaG1HFIgWzJOEWZ7MvpK++odZufgYR50Rw==",
|
||||
"license": "MIT",
|
||||
"engines": {
|
||||
"node": ">=10.0.0"
|
||||
|
|
|
|||
|
|
@ -91,19 +91,14 @@
|
|||
"commander": "^13.1.0",
|
||||
"hono": "^4.12.18",
|
||||
"ink": "^6.0.0",
|
||||
"ink-form": "^2.0.1",
|
||||
"ink-scroll-view": "^0.3.6",
|
||||
"open": "^10.2.0",
|
||||
"pino": "^9.14.0",
|
||||
"react": "^19.2.4",
|
||||
"react": "^19.0.0",
|
||||
"undici": "^8.5.0",
|
||||
"vite": "^8.1.5",
|
||||
"yaml": "^2.9.0",
|
||||
"zod": "^4.4.3"
|
||||
},
|
||||
"overrides": {
|
||||
"ink-select-input": "^6.2.0"
|
||||
},
|
||||
"engines": {
|
||||
"node": ">=22.19.0"
|
||||
},
|
||||
|
|
|
|||
Loading…
Reference in a new issue