fix(tui): bundle React-rendering deps so a consumer install can't split React
`ink-form` and `ink-scroll-view` were externalized by the TUI bundler and
both declare a loose `react` peer (">=18"). Installed under a project that
has its own React 18, npm hoists them to that project's root — where the
peer is satisfied — while the Inspector's React 19 nests underneath. The
bundle then renders through React 19 while those two call hooks on React
18, whose dispatcher is null, so opening a tool test form (or any scroll
view) dies with "Cannot read properties of null (reading 'useState')".
Inline them instead: their `import "react"` is emitted into the bundle and
so resolves from the build directory exactly like the bundle's own, with
npm's placement no longer part of the decision. This also pins their
transitive deps to what this repo resolved — notably `ink-select-input@6`
via `overrides`, which npm ignores for a package installed as a dependency,
so consumers were getting the React-18-era v5.
`ink` stays external: it can't be bundled (its CJS `signal-exit@3` fails
ESM interop with `Dynamic require of "assert"`) and doesn't need to be —
its ">=19" peer keeps npm from hoisting it next to an unusable React.
Both packages are dropped from the root `dependencies`, since the tarball
now ships their code rather than having consumers install them.
`clients/tui/__tests__/tsupConfig.test.ts` is the durable guard: the
failure is invisible in this repo and in every smoke (a dev install has
one React) and only appears once the package is installed under another
project, so the invariant is asserted against the config instead — every
dependency declaring a `react` peer must be bundled unless exempted.
Closes #1952
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WLYmVPVLdeYpE6aMTUsCks
This commit is contained in:
parent
b737360088
commit
eb5bba62ed
3 changed files with 185 additions and 116 deletions
|
|
@ -106,3 +106,35 @@ 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.
|
||||
|
||||
### 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 next to a React satisfying its own peer range.
|
||||
`ink-form` and `ink-scroll-view` both accept `react: ">=18"`, so a project that
|
||||
installs the Inspector alongside its own React 18 satisfies them: they hoist to
|
||||
that project's root while the Inspector's React 19 nests underneath. The bundle
|
||||
then renders through React 19 while those two call hooks on React 18, whose
|
||||
dispatcher is null — so opening a tool test form (or any scroll view) dies with
|
||||
`TypeError: Cannot read properties of null (reading 'useState')`.
|
||||
|
||||
Bundling them removes npm from the decision: their `import "react"` is emitted
|
||||
into `build/index.js`, so it resolves from the build directory exactly like the
|
||||
bundle's own. It also pins their transitive deps to what *this* install
|
||||
resolved — notably `ink-select-input@6` via the `overrides` entry, since npm
|
||||
ignores a dependency's `overrides` and a consumer install would otherwise pull
|
||||
the React-18-era v5 that `ink-form` asks for.
|
||||
|
||||
`ink` itself stays external: it can't be bundled (its CJS `signal-exit@3`
|
||||
dependency fails ESM interop with `Dynamic require of "assert" is not
|
||||
supported`), and it doesn't need to be — its `react` peer is `">=19"`, which
|
||||
keeps npm from hoisting it next to a React the Inspector couldn't also use.
|
||||
|
||||
`__tests__/tsupConfig.test.ts` enforces this: every dependency declaring a
|
||||
`react` peer must be in `noExternal` unless it is listed as external by design.
|
||||
Add a React-rendering dependency, and that test tells you to bundle it.
|
||||
|
|
|
|||
130
clients/tui/__tests__/tsupConfig.test.ts
Normal file
130
clients/tui/__tests__/tsupConfig.test.ts
Normal file
|
|
@ -0,0 +1,130 @@
|
|||
/**
|
||||
* 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 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");
|
||||
}
|
||||
|
||||
/**
|
||||
* The one React-rendering dependency deliberately left external.
|
||||
*
|
||||
* `ink` cannot be bundled — its CJS `signal-exit@3` dependency fails ESM
|
||||
* interop ("Dynamic require of \"assert\" is not supported") — and does not
|
||||
* need to be: its `react` peer is ">=19", so npm cannot hoist it next to a
|
||||
* React the Inspector itself could not use.
|
||||
*/
|
||||
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,
|
||||
);
|
||||
}
|
||||
|
||||
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 can be bundled", () => {
|
||||
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 react itself external, as the single shared instance", () => {
|
||||
expect(external).toContain("react");
|
||||
expect(noExternal).not.toContain("react");
|
||||
});
|
||||
|
||||
it("documents each React-rendering package left external", () => {
|
||||
for (const name of EXTERNAL_BY_DESIGN) {
|
||||
expect(external, `${name} is external by design`).toContain(name);
|
||||
}
|
||||
});
|
||||
|
||||
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);
|
||||
}
|
||||
});
|
||||
});
|
||||
|
|
@ -15,12 +15,28 @@ 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 them is what guarantees that:
|
||||
// an inlined package's `import "react"` is emitted into build/index.js and so
|
||||
// resolves from *this* directory, exactly like the bundle's own — no consumer
|
||||
// install layout can point it somewhere else (#1952). Left external, npm is
|
||||
// free to hoist them next to a *different* React: `ink-form` and
|
||||
// `ink-scroll-view` declare a loose `react` peer (">=18"), so a consumer
|
||||
// project holding React 18 satisfies it and gets them hoisted to its own root
|
||||
// while the Inspector's React 19 nests under it — 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. `__tests__/tsupConfig.test.ts` guards this list.
|
||||
noExternal: [/^@inspector\/core/, "ink-form", "ink-scroll-view"],
|
||||
external: [
|
||||
// `react` is deliberately external — it is the single instance everything
|
||||
// above resolves to, from this build directory.
|
||||
"react",
|
||||
// `ink` stays external too: it cannot be bundled (its CJS `signal-exit@3`
|
||||
// dependency fails ESM interop with "Dynamic require of \"assert\""), and
|
||||
// it does not need to be — its `react` peer is ">=19", which keeps npm
|
||||
// from hoisting it next to a React the Inspector could not also use.
|
||||
"ink",
|
||||
"ink-form",
|
||||
"ink-scroll-view",
|
||||
"open",
|
||||
"commander",
|
||||
"pino",
|
||||
|
|
|
|||
Loading…
Reference in a new issue