Repository navigation
fix(tui): bundle React-rendering deps so a consumer install can't split React #1983
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
eb5bba6
fix(tui): bundle React-rendering deps so a consumer install can't spl…
cliffhall 3c1bbbc
fix(tui): correct ink-form's misspelled incomplete-form hint
cliffhall 39575ff
test(tui): drive the ink-form patch through the plugin, and drop the …
cliffhall 9fd5bc1
chore: drop the orphaned root override, and correct the TUI coverage …
cliffhall 94a227b
fix(tui): widen the root react range so an external ink can't split R…
cliffhall File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,167 @@ | ||
| /** | ||
| * 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, | ||
| fixInkFormIncompleteHint, | ||
| } 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("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 submitButton = readFileSync( | ||
| path.join( | ||
| clientDir, | ||
| "node_modules", | ||
| "ink-form", | ||
| "lib", | ||
| "SubmitButton.js", | ||
| ), | ||
| "utf8", | ||
| ); | ||
|
|
||
| const patched = fixInkFormIncompleteHint(submitButton, "SubmitButton.js"); | ||
| expect(patched).toContain(INK_FORM_INCOMPLETE_HINT.fixed); | ||
| expect(patched).not.toContain(INK_FORM_INCOMPLETE_HINT.typo); | ||
|
cliffhall marked this conversation as resolved.
Outdated
|
||
| }); | ||
|
|
||
| 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); | ||
| } | ||
| }); | ||
| }); | ||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.