ESM migration: convert lib batch 3 (printing and ui) - #9069
ESM migration: convert lib batch 3 (printing and ui)#9069eventualbuddha wants to merge 8 commits into
Conversation
|
Parking this in draft — all 7 app-backend test suites fail, and the cause is test infrastructure rather than the libraries themselves. Recording the diagnosis so far. Symptom. Evidence (reproduces locally, so no CI round-trips needed):
Fixes tried and ruled out, all at the test-config layer:
That the config layer cannot fix it suggests the problem is module-system consistency: Worth stressing: this is test-harness breakage, not product breakage. The converted libraries are sound — all 255 built Next step is probably to extend this PR to convert |
|
Resolved — pushed as Tagging the module and logging from both sides showed one instance with two views: Each Fix: load printing inside the hook with Measured on admin-backend, the hardest case: eager import → 31 tests failing; Two options ruled out with measurements rather than argument:
Follow-up worth considering: those same setup files also import |
c106152 to
8960c69
Compare
8960c69 to
d8581a7
Compare
2a62e7f to
dcf7d98
Compare
dcf7d98 to
2255d77
Compare
2255d77 to
f32f12b
Compare
f32f12b to
4324ec7
Compare
f38ed03 to
976bf26
Compare
865d9dd to
b517172
Compare
Codemod: `"type": "module"` + `exports`, explicit `.js` specifiers, `import.meta.dirname`. Manual work: - **The three `scripts/` CLIs** (`printer`, `generate-m404n-ppd`, `render-tally-report`) import the compiled output instead of installing `esbuild-runner`. That hook is a CommonJS `require` hook: it can neither load this package's ESM sources nor resolve the `.js` specifiers they now use. These are extensionless files, so node takes their module system from this package.json and was already parsing them as ESM the moment `"type": "module"` landed. Two of them ran TypeScript that lived in `scripts/`, which `tsconfig.build.json` does not compile, so there was nothing built to import. Those two modules move to `src/scripts/` to be compiled like everything else — with a matching `coverage.exclude`, since they sat outside `src/` before and so were never counted. The `eslint` `no-console` override for `scripts/**` extends to their new home. - `src/render.test.tsx` took `styled` as a default import. styled-components v5 ships CommonJS only, so node's ESM interop yields `module.exports` and `styled.div` is `undefined`; vitest hands back `styled` itself. New `test/styled.ts` normalizes both shapes (same fix as `libs/hmpb/src/styled.ts` — here it is test-only, so it lives in `test/`). - `.lintstagedrc.js` renamed to `.cjs`. Verified: `tsc --build` and `eslint` clean; `vitest run` 74/74 and coverage still 100%/99%; `build/index.js` imports under plain `node`; all three CLIs print usage; and re-running `generate-m404n-ppd` reproduces the committed PPD byte for byte. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TPPUVQen9XKKg9E4JC1yaU
Preparation for converting ui to ESM, landed separately so it can be reviewed and validated on its own: this commit is a no-op for the current CommonJS build, and every check passes with or without it. styled-components v5 ships CommonJS only. Under node's ESM interop `import styled from 'styled-components'` evaluates to `module.exports`, so `styled.div` is `undefined` and every template literal throws at import time — which matters here because ui renders server-side inside backends, not only in the browser. Vitest and Vite hand back `styled` itself instead (vitest applies its own `interopDefault`; Vite resolves the package's ESM build), so a fix written for one loader breaks the other. New `src/styled.ts` accepts either shape, and the 115 modules that used the default import now take `styled` from there. Its type comes from the package's own `StyledInterface` rather than `typeof styledDefault.default`, because CommonJS and ESM type resolution disagree about what a default import of a CommonJS module is, and this file has to type-check under both — before the flip and after. Named imports are untouched (`css`, `keyframes`, `ThemeProvider`, `DefaultTheme`, …): node's CommonJS named-export detection resolves those correctly, and the type-only ones never reach runtime. Verified: `tsc --build`, `eslint` and `stylelint` clean; `vitest run` 939 passed / 2 skipped across 168 files. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TPPUVQen9XKKg9E4JC1yaU
Codemod: `"type": "module"` + `exports`, 1385 explicit `.js` specifiers and 13 `import.meta` globals across 406 files. (The styled-components interop this needed landed in the previous commit.)
Manual work, grouped by what broke:
**Build configuration.** `i18next-parser.config.js` becomes ESM. It cannot simply be renamed: i18next-parser looks for `i18next-parser.config.{js,mjs,json,ts,yaml,yml}` and would not find a `.cjs`, and it loads the config with `import()`, so `export default` is what works. Its header comment already explained why it is plain JavaScript rather than TypeScript (a `.ts` config races on a temp file during concurrent monorepo builds); that reasoning is unchanged and now records the ESM part too. This one matters more than the usual config rename — `build:app-strings-catalog` runs as part of `build:self`, so it fails *every* build of ui, and therefore of the whole repo.
`.lintstagedrc.js` and `.stylelintrc.js` renamed to `.cjs`. `.storybook/main.ts` exports with `export default` instead of `module.exports`, and its stale `@ts-expect-error` on the `vite` import is gone (the error it suppressed does not occur under ESM resolution).
**The two `scripts/` CLIs** import compiled output instead of installing `esbuild-runner`, which is a CommonJS `require` hook and can neither load ESM sources nor resolve the `.js` specifiers they now use. Their TypeScript moved to `src/scripts/` so it is compiled at all (`tsconfig.build.json` only covers `src`), with a matching `coverage.exclude` — those modules sat outside `src/` before and so were never counted. Both write files back into `src/`, so their paths are now resolved relative to the compiled location; that is called out where it happens. `build:app-strings-catalog` compiles first, so it still works when lint-staged invokes it on its own.
**Three more CommonJS dependencies whose default export ESM and bundlers disagree about**, each fixed where it is used:
- `qrcode.react` — `import { QRCodeSVG }` type-checks but fails to *load* under node ESM: node's named-export detection cannot see the package's exports, and only the namespace's `default` (i.e. `module.exports`) carries them. Vite and vitest do the reverse. `src/qrcode_react.ts` takes whichever has the component and re-exports it as `QrCodeSvg`.
- `i18next` — declares one `types` entry for both its CommonJS and ESM builds, so TypeScript models the default import as `module.exports` even though node resolves the ESM build, whose default *is* the instance.
- `@testing-library/user-event` v13 — CommonJS only, so the default import is `module.exports` under node while vitest unwraps it. `src/user_event.ts` normalizes it for the 58 test files plus `accessible_controllers/test_utils.tsx`; it lives in `src/` rather than `test/` because that test-utils module is compiled into the build and so cannot import from outside `rootDir`, which needs a narrow `import/no-extraneous-dependencies` exception.
**Specifier fixes the codemod could not make.** `./normalize.css` is a TypeScript module named `normalize.css.ts`, and the codemod skips anything ending in `.css` on the assumption it is a stylesheet — so it needed `.js` by hand. `@testing-library/jest-dom/matchers` needed `.js` too: that package has no `exports` map, so under node16 the subpath is resolved as a file path and needs its extension. `src/fonts/generate_font_awesome_styles.ts` used `require.resolve`, now `createRequire(import.meta.url).resolve`. And one test's deep `@votingworks/types/src/auth/...` import now uses the public `DippedSmartCardAuth` namespace.
Verified: `tsc --build`, `eslint` and `stylelint` clean, including the app-strings-catalog check; `vitest run` 939 passed / 2 skipped across 168 files with coverage debt unchanged from baseline; **all 255 built modules import successfully under plain `node`** (a whole-`build/` sweep, which is what caught the `qrcode.react` and `require.resolve` failures — neither is visible to `tsc` or to vitest); both CLIs run and reproduce their generated files byte for byte.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TPPUVQen9XKKg9E4JC1yaU
Converting printing and ui to ESM broke `vi.mock("@votingworks/utils")` in every app-backend test suite: `libs/backend` kept the real `isFeatureFlagEnabled`, so `SKIP_ELECTION_PACKAGE_AUTHENTICATION` never took effect, election-package authentication ran for real, and 7 suites failed on `Missing required VX_MACHINE_TYPE env var`.
It is a load-ordering problem, not duplicate modules. Tagging the module and logging from both sides shows one instance with two views:
backend sees utils instance: build:3ve7d | mocked? false
test file utils = build:3ve7d | mocked? true
Each `test/setupTests.ts` imported `@votingworks/printing` at module scope for a single `cleanupCachedBrowser()` call. Setup files run before the test file, hence before any `vi.mock`. While printing was CommonJS it was externalized and never entered vitest's module runner; as ESM it is processed by the runner, so importing it instantiates a chunk of the dependency graph pre-mock, and those modules keep their real bindings.
Fixed by loading printing inside the hook instead, via `vi.importActual` — which additionally resolves printing's own dependencies unmocked, and which returns the same module instance when nothing is mocked, so the browser it cleans up is still the one the tests created.
`admin-backend`'s `multi_station_config.test.ts` needed one more change: it replaced `node:fs` wholesale, and `importActual` unmocks the module named, not its dependencies — so printing's import-time read of its printer configs got the stub and threw on `"enable"`. Its `readFileSync` stub now defaults to the real implementation, which individual tests still override.
Note this is why aliasing packages to source cannot fix this class of failure — aliases address module identity, and this is timing. Aliasing `utils` to source, aliasing `printing`, and `server.deps.inline: [/@votingworks\//]` were all measured and left the mock unapplied.
Verified: full suites pass for all eight backends and bmd-ballot-fixtures — admin 462/462 across 44 files (31 were failing), print 69/69, central-scan 121/121, mark 128/128, mark-scan 161/161, scan 226/226, design 192/192, pollbook 218/218 — and `lint` is clean in all nine packages.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TPPUVQen9XKKg9E4JC1yaU
The failure fixed in the previous commit was silent and remote: one module-scope import in a setup file left a mock unapplied in a module nobody edited, in a different package. New `vx/no-esm-workspace-import-in-test-setup` catches it at authoring time, enabled for `setupTests.{ts,tsx}`, `test/setup.ts`, `test/setup_custom_matchers.ts` and `test/set_env_vars.ts`.
The rule reports only packages that are **currently** ESM, by resolving the imported package's `package.json` the way node would — walking up for `node_modules/<name>/package.json` — and checking `"type"`. That matters for two reasons. Today there are no violations, so this adds no churn: every module-scope workspace import left in a setup file is `fixtures`, `image-utils` or `usb-drive`, all still CommonJS, and all harmless while they are externalized rather than run inside vitest's module runner. And it means each package starts being flagged as it converts, so the fix lands in that batch's PR rather than being paid for speculatively now — there are 44 such imports across 36 setup files, some of which (registering matchers with `expect.extend`) need thought rather than a mechanical rewrite.
Type-only imports are not reported, since they are erased. Unresolvable packages and unparseable manifests are not reported either: the rule fails open rather than guessing.
Verified by re-introducing the exact regression — putting the module-scope `@votingworks/printing` import back into `apps/print/backend/test/setupTests.ts` — and confirming `eslint` reports it through the package's own config, with a message naming the `vi.importActual` replacement. Rule tests 10/10; the plugin's suite stays at 483 passing with statements and lines at 100%; `lint` clean for ui, printing and design-backend, the packages whose setup files import the most.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TPPUVQen9XKKg9E4JC1yaU
None of these are covered by CI, which is why they were green while broken. They installed the `esbuild-runner` hook and required TypeScript sources: that hook is CommonJS-only, and it resolves `.js` specifiers literally so it cannot find the `.ts` files those specifiers now point at. They are extensionless files, so node takes their module system from the package and had been parsing them as ESM ever since `"type": "module"` landed.
Eight are fixed. Five now import compiled output directly. Three (`copy-batch`, `copy-sheets`, `simulate-check-ins`) ran TypeScript that lived in `scripts/`, which `tsconfig.build.json` does not compile, so there was nothing built to import — those modules move to `src/scripts/`, with narrow `coverage.exclude` entries because they sat outside `src/` before and so were never counted, and the `no-console` eslint override for `scripts/**` extends to their new home.
Two are deleted rather than ported: `central-scan/backend/bin/{read-qrcode,render-pages}` required `../src/cli/`, which was removed in 8a41def long before this migration, and nothing references them.
VxDesign's two CLIs get the same treatment in its own PR, alongside the startup fix, so that design's conversion is self-contained.
Verified: each of the eight runs and prints its usage or reaches a real argument or environment error; `lint` and `tsc` clean in all five packages; and full suites with coverage pass — central-scan 121/121, plus pollbook, scan, custom-paper-handler and fixture-generators.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TPPUVQen9XKKg9E4JC1yaU
The `bin/` commands now run compiled output rather than transpiling `src` on the fly, so following the README on a fresh checkout fails with `ERR_MODULE_NOT_FOUND` until the package is built. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TPPUVQen9XKKg9E4JC1yaU
Same as the previous batches: #9079 landed the convention and a validate-monorepo check that enforces it. Produced by re-running the codemod. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TPPUVQen9XKKg9E4JC1yaU
b517172 to
4b52e77
Compare
Overview
Library batch 3: printing and ui — the two links that had to come next, and the ones carrying the styled-components work.
printingwas the only library convertible at all (everything else was still blocked), and converting it unblocksui, which in turn unblocksbackend,ballot-encoder,fujitsu-thermal-printerandmonorepo-utilsfor the next batch.Three commits:
libs/uiis the largest package in the repo at 476 TypeScript files, and unlike the frontend libraries it is on node resolution, because its output is loaded by node — backends render reports and ballots through it. That is what makes the interop work below necessary rather than cosmetic.Reviewer's main focus
1. styled-components (commit 2). v5 ships CommonJS only, so under node's ESM interop
import styled from 'styled-components'evaluates tomodule.exports—styled.divisundefinedand every template literal throws at import time. Vitest and Vite hand backstyleditself instead, so a fix written for one loader breaks the other.src/styled.tsaccepts either shape; its type comes from the package's ownStyledInterfacerather thantypeof styledDefault.default, because CommonJS and ESM type resolution disagree about what a default import of a CommonJS module is and this file has to type-check on both sides of the flip.2. Three more dependencies with the same disagreement, each handled where it is used, and each caught a different way:
qrcode.reactimport { QRCodeSVG }type-checks but fails to load — node cannot see the package's named exports; only the namespace'sdefaulthas them, and Vite/vitest are the reversei18nexttypesentry for both builds, so TS models the default asmodule.exportsthough node resolves the ESM buildtsc@testing-library/user-eventv13tsc3. ui's build configuration.
i18next-parser.config.jsbecomes ESM rather than.cjs— i18next-parser looks for.js/.mjs/.json/.ts/.yaml(never.cjs) and loads it withimport(). This one is load-bearing:build:app-strings-catalogruns insidebuild:self, so getting it wrong fails every build of ui and therefore of the repo. The existing comment explaining why the config is JavaScript rather than TypeScript still applies and now records the ESM part too.4. The dev CLIs (three in printing, two in ui) import compiled output instead of installing
esbuild-runner, which is a CommonJSrequirehook that can neither load ESM sources nor resolve the.jsspecifiers those sources now use. Four of them ran TypeScript that lived inscripts/, which the build does not compile, so there was nothing to import; those modules moved undersrc/with a matching narrowcoverage.exclude— they sat outsidesrc/before and so were never counted, and the two ui ones write back intosrc/, which is called out where the paths are resolved.Also worth knowing: two specifier fixes the codemod cannot make.
./normalize.cssis reallynormalize.css.ts, and the codemod skips anything ending in.cssassuming it is a stylesheet.@testing-library/jest-dom/matchersneeds its.jsbecause that package has noexportsmap, so node16 resolves the subpath as a file path.Demo Video or Screenshot
N/A — module-system change, no user-facing behavior change.
Testing Plan
Per package, locally:
tsc --buildeslint/stylelintvitest runnodegenerate-m404n-ppdreproduces the committed PPD byte for bytegenerate-font-awesome-stylesand the strings catalog regenerate identicallyThe whole-
build/import sweep is the check that matters most here, and it is new in this batch. The unit tests run under vitest, which resolves modules the way Vite does — the opposite side of every interop question in this PR — so they cannot see a node-only load failure. Sweeping all 255 built modules under plainnodeis what caughtqrcode.reactand therequire.resolve, neither of whichtscor vitest flags.Repo-wide on this branch:
type-checkpasses for every dependent of both packages, andscript/validate-monorepois clean. CI covers the rest, including the frontend builds (Vite consuming ESM ui) and the integration tests that render through printing and ui inside real backends.Checklist
🤖 Generated with Claude Code