Skip to content

build(COMPASS-28): move to inquirer 11 - #2723

Draft
timdawborn wants to merge 1 commit into
compass-28-express-5from
compass-28-inquirer-11
Draft

build(COMPASS-28): move to inquirer 11#2723
timdawborn wants to merge 1 commit into
compass-28-express-5from
compass-28-inquirer-11

Conversation

@timdawborn

@timdawborn timdawborn commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Ticket

COMPASS-28 — Bump spot's NodeJS minimum from 18 to 22

PR 6 of 9. Staying in draft until the whole chain is verified.

Stacked on #2722#2721#2720#2719. This PR's own commit is 7c69fad.

Why 11 and not 14

inquirer ^8.1.1 → ^11.1.0, @types/inquirer removed (the package ships its own declarations from 9 on).

11 is the ceiling for this package, not a cautious choice. inquirer 12, 13 and 14 are all "type": "module", and spot is CommonJS throughout — module: "commonjs", bin/run is a require(), ts-jest runs the suite as CJS. 10 and 11 are the last dual-published majors:

inquirer type
9.3.8 module
10.2.2 commonjs (dual)
11.1.0 commonjs (dual)
12.11.1 / 13.4.3 / 14.0.2 module

So Dependabot #2637 (→ 14) cannot merge without converting the whole package to ESM, which is a separate project. Worth closing with that reason so it stops being re-proposed weekly.

Three API changes between 8 and 11

1. The named prompt export is gone.

$ node -e "const m=require('inquirer'); console.log(Object.keys(m), typeof m.prompt)"
[ 'createPromptModule', 'default' ] undefined

So import { prompt } becomes import inquirer + inquirer.prompt(...), and the spec's jest.mock("inquirer", () => ({ prompt: jest.fn() })) has to stand in for the default export instead.

2. message is now required — and it is user-visible.

inquirer 8 derived it in lib/prompts/base.js:

if (!this.opt.message) {
  this.opt.message = this.opt.name + ':';
}

So these prompts already read Generator:, Language: and Output destination: on screen. Each is now spelled out with that exact text, and inquirer 11 renders ${prefix} ${message} with nothing appended (checked in @inquirer/select and @inquirer/input), so what a user sees is unchanged rather than approximately unchanged.

3. input was the default question type until 9 and is explicit now.

The suite could not have caught a broken inquirer

generate.spec.ts mocks inquirer entirely, so it passes against a module that fails to load at all — which is exactly the failure mode an ESM-only major would produce. So I drove the real thing through a pty:

? Generator: (Use arrow keys)
❯ json-schema
  openapi2
  openapi3
  raw

All three prompts render, accept input, and the command exits 0. Taking the first choice at each prompt produced an artifact identical to the same run with flags:

diff -r /tmp/pr6-out /tmp/pr6-flags   # empty

That also confirms the prompt-to-value mapping still lands on the right generator and language, not just that the prompts appear.

How this was verified

Locally on Node 22.23.2, all exit 0:

Check Result
pnpm build 0
pnpm test 0 — 54 suites, 555 tests, 44 snapshots
pnpm lint:check 0
pnpm build-docs 0
docker build + check-image-parity 0 — all checks passed
real interactive run via pty exit 0, output matches the flag-driven run

inquirer is a runtime dependency because generate prompts, so it ships in the image. Parity's seven generate cases passing is the proof that require("inquirer") resolves under CJS inside the image — the image also reports @airtasker/spot/2.1.0 linux-arm64 node-v22.23.2, confirming the base-image pin from #2718.

@timdawborn
timdawborn changed the base branch from master to compass-28-express-5 August 20, 2026 00:11
@timdawborn

Copy link
Copy Markdown
Contributor Author

Automated review — /pr-review-toolkit:review-pr

Agents run: code-reviewer, pr-test-analyzer. Scoped to this PR's own commit (7c69fad).

🔴 Critical (0)

None.

🟡 Important (3)

1. cli/src/commands/generate.ts:130 — the comment's causal claim is false. // \input` was the default type until inquirer 9; it is explicit now.inquirer 11's legacy runner still defaults it —dist/cjs/ui/prompt.js, prepareQuestion: type: question.type in this.prompts ? question.type : 'input'. The real reason typemust be spelled out is **TypeScript**:QuestionWithGettersdeclarestype` non-optional. The same wrong claim is in the commit message. A reader trusts it and concludes the runtime fallback is gone.

2. cli/src/commands/generate.spec.ts:9-12 — the suite is blind to the failure mode this PR exists to avoid. The jest.mock factory never touches the real module, so an ESM-only inquirer that cannot load under CJS would leave every test green. ^11.1.0 encodes the ceiling today; a Dependabot major rewrites it and nothing objects. Five lines fix it:

test("inquirer is loadable as CommonJS", () => {
  const actual = jest.requireActual("inquirer");
  expect(typeof (actual.default ?? actual).prompt).toBe("function");
  expect(require("inquirer/package.json").type).not.toBe("module"); // 12+ is ESM-only
});

The pkg.type assertion is the durable one — it fails at the point of the bump and names the reason.

3. Nothing asserts the prompt arguments. Drop message: from any of the three call sites and inquirer 11 throws at runtime while toHaveBeenCalledTimes(3) still passes. The comments at :78-80 and :130 claim an invariant no test isolates:

expect(promptMock.mock.calls.map(([q]) => [q.name, q.message, q.type])).toEqual([
  ["Generator", "Generator:", "list"],
  ["Language", "Language:", "list"],
  ["Output destination", "Output destination:", "input"]
]);

🔵 Suggestions (2)

  • The three migration comments are change-narration ("inquirer 8 derived the label from name", "it is explicit now", "inquirer 9 dropped the named prompt export`"). The commit message already carries all three; the repo's rules keep them out of the file. Deleting them also disposes of finding 1.
  • type: "list" is deprecated in inquirer 11 (/** @deprecated \list` is now named `select` */). Free to switch to "select"` while these objects are already open.

✅ Verified sound

  • The message strings are exact. inquirer 8 lib/prompts/base.js:41message = name + ':'; inquirer 11 joins [prefix, message, …] with nothing appended. "Generator:", "Language:", "Output destination:" reproduce the old text precisely. One caveat the description overstates: the prefix colour changes (green ? → blue ?), so "what a user sees does not change" is true of text, not appearance.
  • The __esModule: true mock shape is right — with esModuleInterop, __importDefault returns the mock unchanged and .default.prompt is the jest.fn.
  • 11 over 10 is immaterial to this diff — all three changes are forced at the 8→9 boundary, and ^11.1.0 correctly caps below ESM-only 12.
  • A pty test in jest is not worth it (agrees with the choice made here): needs a build step, a native dep, and ANSI/timing assertions that go flaky. scripts/check-image-parity is the natural host if an interactive smoke is ever wanted.

Recommended action

  1. Fix or delete the type comment — it is actively misleading.
  2. Add the CJS-loadability assertion; it is the cheap guard for the one risk that pinning at 11 is managing.
  3. Assert the prompt arguments so the message invariant has a test.

`inquirer` to ^11.1.0, and `@types/inquirer` removed — the package has
shipped its own declarations since 9.

11 is the ceiling, not the latest. inquirer 12, 13 and 14 are all
`"type": "module"`, and this package is CommonJS throughout: `module` is
`commonjs`, `bin/run` is a `require`, and ts-jest runs the suite as CJS. 10
and 11 are the last dual-published majors. Going further needs the whole
package converted to ESM, which is not this change.

The named `prompt` export is gone; only `createPromptModule` and a default
carrying `prompt` remain. The command imports the default now, and the
spec's `jest.mock` stands in for the default export rather than a named one.

`message` and `type` are non-optional on inquirer's question type, so both
are spelled out. The labels are the text inquirer 8 produced for the same
prompts, so what a user reads does not change. The compiler requires them
to be present but cannot check what they say, and a wrong label compiles
cleanly — so the three are asserted in the spec, mutation-checked.

Nothing else in the suite touches the real module: it is mocked wholesale,
so it would pass against an inquirer that cannot be loaded at all, which is
the shape an ESM-only major takes here. One case now resolves it for real
and reads the manifest off disk — the `exports` map does not expose
`./package.json` — so a move to an ESM-only version fails at the bump
rather than in a consumer's terminal.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@timdawborn

Copy link
Copy Markdown
Contributor Author

Addressed in c75852c.

Two of the three findings held; one did not, and I want to flag it rather than quietly act on it.

Finding 1 (the type comment) — sound. Confirmed at inquirer/dist/cjs/ui/prompt.js:176: type: question.type in this.prompts ? question.type : 'input'. The runtime default is still there; it is the non-optional declared type that forces it. Comment removed rather than reworded.

Finding 2 (CJS loadability) — sound, but the suggested test does not work. require("inquirer/package.json") throws ERR_PACKAGE_PATH_NOT_EXPORTED — inquirer's exports map does not expose it, so that assertion would fail for the wrong reason. Implemented by resolving the entry and reading the manifest from disk instead.

Finding 3 (prompt arguments) — premise unsound. Dropping message does not leave the suite green: message and type are both non-optional on inquirer's question type, and removing either produces TS2769. The compiler already enforces presence.

It is still worth an assertion, for a different reason: the compiler checks presence, not text, and message: "Pick a generator:" compiles fine. Verified — that mutation gives 0 type errors and fails the new test. So the assertion pins the label text, which is what a user actually reads.

Also dropped the change-narration comments per the Suggestions tier, since the same edits touched those lines.

@timdawborn
timdawborn force-pushed the compass-28-inquirer-11 branch from 7c69fad to c75852c Compare August 20, 2026 03:31
@timdawborn

Copy link
Copy Markdown
Contributor Author

Automated review, round 2 — /pr-review-toolkit:review-pr

Agents: code-reviewer, pr-test-analyzer. Scoped to the revised commit c75852c.

Round 1's three findings: two hold, one I fixed badly. Both agents landed on the same defect independently.

🔴 Critical (1)

generate.spec.ts:118-125 — the loadability test I added in round 1 is vacuous on the very next major, and asserts against the wrong file.

The walk goes up from require.resolve("inquirer") until it finds a package.json. On inquirer 11 that lands on inquirer's own manifest. On inquirer 12 it does not — 12 ships dist/commonjs/package.json, a two-line stub. Installed 12.9.6 and ran the walk verbatim:

resolved entry : .../inquirer@12.9.6/.../inquirer/dist/commonjs/index.js
walk stopped at: .../inquirer/dist/commonjs/package.json
its contents   : {"type":"commonjs"}
my assertion   : PASS
real inquirer type: module

So it asserts against a stub that hardcodes commonjs and can never fail, while the real manifest says module. It is layout-dependent, tests a proxy (a type field) rather than the precondition that matters (does it load under CJS), has no manifest.name === "inquirer" guard, and no root-termination guard — path.dirname("/") === "/" loops forever if nothing is found.

jest.requireActual("inquirer") on the line above already is the CJS-loadability check: an agent confirmed Jest's resolver does not honour Node 22's require(esm), so a genuinely ESM-only inquirer throws there first. Drop lines 118-125, or add the name assertion so a stub hit fails loudly.

And the commit message is wrong on the reason for the ceiling. "10 and 11 are the last dual-published majors" — false. inquirer 12 has a require condition and loads cleanly under CJS. Only 13 and 14 are ESM-only. So ^11 is more conservative than it needs to be, and the stated justification does not hold for 12. Either move to ^12 or correct the reason.

🟡 Important (2)

Nothing durable pins the ceiling. No ignore entry in .github/dependabot.yml, no note in AGENTS.md, no comment at the version range. git grep -i inquirer outside the diff returns one line — the range itself. Dependabot will propose 14 weekly, and the only thing stopping it is requireActual failing.

generate.spec.ts:141 — dead spy. It mocks console.log, but in this PR the command still logs through oclif v1's this.logprocess.stdout.write. Generated /…/api.yml leaks to test output; the sibling test mocks process.stdout.write correctly. (This becomes right only after #2730 moves to core 4, which is a good illustration of why a stacked fix needs checking against its own base.)

🔵 Suggestions (2)

  • default: "." on the output prompt is unconstrained. Deleting it leaves tsc clean and all 7 tests passing. Add question.default to the asserted tuple, expecting [undefined, undefined, "."].
  • Overlap between the new label test and "prompts for the missing flags when there is a terminal": the toEqual on a 3-element array subsumes toHaveBeenCalledTimes(3). Drop the latter, or fold the file-exists assertion into the label test.

✅ Round-1 fixes that hold

  • The false type comment is gone — correctly, and confirmed again at dist/cjs/ui/prompt.js:176.
  • The label assertion is load-bearing, mutation-proven twice independently: "Language:""Language" gives 0 type errors and fails the toEqual. And round 1's premise was indeed wrong — deleting any message or the type is a real TS2769, so the compiler covers presence and the test covers text. That split is stated accurately in the comment.
  • createPromptModule has zero call sites; no coverage owed.

Recommended action

  1. Drop the manifest walk — it cannot fail on the case it was written for, and requireActual already covers it.
  2. Correct the ceiling reason, and decide whether ^12 is now the right range.
  3. Fix the dead spy; add the default assertion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant