Skip to content

fix: make the tsd type tests actually run - #322

Merged
cryptodev-2s merged 1 commit into
mainfrom
migrate/pr2h-fix-tsd
Sep 10, 2026
Merged

fix: make the tsd type tests actually run#322
cryptodev-2s merged 1 commit into
mainfrom
migrate/pr2h-fix-tsd

Conversation

@cryptodev-2s

@cryptodev-2s cryptodev-2s commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Top of stack #315, on #321.

yarn test:types runs bare tsd, which finds no test files at all. It produces no output and exits 0 regardless, so the 460 lines of type assertions across src/*.test-d.ts have not been checking anything.

Proof, before this change:

$ printf 'expectAssignable<Hex>(999);' >> src/hex.test-d.ts
$ yarn test:types
$ echo $?
0

Pointing tsd at the files explicitly reports it properly and exits 1:

✖  Argument of type 999 is not assignable to parameter of type `0x${string}`.

Cause

tsd resolves test files relative to the types field, which has pointed at ./dist/index.d.cts since ts-bridge was adopted in #182 (2024-04-23). tsd 0.29 does not resolve .d.cts, and the tsd.directory setting does not compensate. 26 releases have shipped since, all with this check silently passing.

Fix

- "test:types": "tsd"
+ "test:types": "tsd --files 'src/*.test-d.ts'"

Fixing the invocation rather than the types field, since the field is correct for consumers and only tsd is confused by it.

All four files pass once actually executed, so none of them needed changing. yarn test:types now exits 1 on a bad assertion and 0 when clean.

Relevance to the migration

This was found while working out what to do with tsd in Phase B, since core has no way to run it. Worth knowing the honest baseline before deciding: these assertions have been dormant for 17 months, so whatever we do with them in core, we are not losing coverage we currently have.


Note

Low Risk
Only the npm script for type testing changed; runtime package behavior and public types are unchanged.

Overview
yarn test:types was a no-op: bare tsd never picked up the src/*.test-d.ts suites (because of how it resolves files vs the package types entry), so type assertions could fail silently while CI still exited 0.

The script now runs tsd --files 'src/*.test-d.ts', so those assertions actually execute and yarn test:types fails when a type check is wrong. No changes to the test files themselves were required once they were being run.

Reviewed by Cursor Bugbot for commit b934063. Bugbot is set up for automated code reviews on this repo. Configure here.

@cryptodev-2s
cryptodev-2s removed this pull request from stack #329 September 9, 2026 11:46
@cryptodev-2s
cryptodev-2s added this pull request to stack #331 September 9, 2026 11:47
@cryptodev-2s
cryptodev-2s force-pushed the migrate/pr2h-fix-tsd branch 2 times, most recently from 1342ebd to ebe3626 Compare September 9, 2026 16:32
Mrtenz
Mrtenz previously approved these changes Sep 10, 2026
@cryptodev-2s
cryptodev-2s dismissed Mrtenz’s stale review September 10, 2026 10:15

The merge-base changed after approval.

@cryptodev-2s
cryptodev-2s removed this pull request from stack #331 September 10, 2026 10:40
@cryptodev-2s
cryptodev-2s added this pull request to stack #335 September 10, 2026 10:41
@cryptodev-2s
cryptodev-2s requested a review from Mrtenz September 10, 2026 10:46
Base automatically changed from migrate/pr2g-ethereumjs to main September 10, 2026 13:15
`yarn test:types` runs bare `tsd`, which finds no test files at all. It
produces no output and exits 0 no matter what, so the 460 lines of type
assertions in src/*.test-d.ts have not been checking anything.

Proof: appending `expectAssignable<Hex>(999)` to hex.test-d.ts still exits
0. Pointing tsd at the files explicitly reports it correctly and exits 1.

The cause is the `types` field. tsd resolves test files relative to it, and
it has pointed at ./dist/index.d.cts since ts-bridge was adopted in #182 on
2024-04-23, which tsd 0.29 does not resolve. The `tsd.directory` setting
does not compensate. There have been 26 releases since, all with this check
silently passing.

Fixing the invocation rather than the `types` field, since the latter is
correct for consumers and only tsd is confused by it. All four existing
files pass once actually executed, so nothing needed changing in them.
@cryptodev-2s
cryptodev-2s merged commit 106371c into main Sep 10, 2026
22 checks passed
@cryptodev-2s
cryptodev-2s deleted the migrate/pr2h-fix-tsd branch September 10, 2026 13:21
cryptodev-2s added a commit that referenced this pull request Sep 10, 2026
Replaces #324, which GitHub auto-closed as merged during a stack reorder
when its head briefly became an ancestor of its base. The changes never
reached `main`; this carries the same two commits.

Mirrors
[MetaMask/core#9976](MetaMask/core#9976), the
bottom of core's foundational stack.

| | Before | After |
| --- | --- | --- |
| `engines.node` | `^18.18 \|\| ^20.14 \|\| >=22` | `^22.14.0 \|\| ^24`
|
| `@types/node` | `~18.18.14` | `^22.13.14` |
| CI matrix | 18, 20, 22 | 22, 24 |

`constraints.pro` is updated so `yarn constraints` enforces the new
range. Core makes the `@types/node` bump in this same PR rather than
with its TypeScript change, since the types track the supported runtime.

## Position in the stack

This now sits **below** the ESM conversion (#323), so everything up to
and including this PR is still a hybrid CJS/ESM build:

```
main → … → #322 → this → #323 (ESM only) → #325 (TypeScript)
```

That matters for testing. A preview build from here still resolves
through `main: ./dist/index.cjs` with the `require` condition intact, so
it can be consumed by `metamask-extension` as-is. Everything below the
ESM cut can therefore be verified against a real downstream consumer
before the breaking change lands.

<!-- CURSOR_SUMMARY -->
---

> [!NOTE]
> **Medium Risk**
> Semver-breaking runtime requirement may block consumers still on Node
18 or 20; in-repo changes are mostly policy, CI, and test cleanup with
limited logic impact.
> 
> **Overview**
> **Breaking:** supported Node is now `^22.14.0 || ^24` instead of
18/20/22. That is enforced in `package.json`, `constraints.pro`, and
documented under Unreleased in `CHANGELOG.md`.
> 
> CI **prepare**, **test**, and **compatibility-test** jobs now run on
Node **22.x** and **24.x** only (18 and 20 dropped from the matrix). Dev
typings move from `@types/node` ~18 to **^22.13.14**, with matching
`yarn.lock` updates.
> 
> Test and lint tooling align with the new floor: ESLint comments for
`n/no-unsupported-features/node-builtins` reflect that global `crypto`
is expected on 22+, and `hashing.test.ts` drops the Node 18 `webcrypto`
polyfill/`beforeEach` setup—tests assume `globalThis.crypto.subtle`
exists.
> 
> <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit
ba81eca. Bugbot is set up for automated
code reviews on this repo. Configure
[here](https://www.cursor.com/dashboard/bugbot).</sup>
<!-- /CURSOR_SUMMARY -->
cryptodev-2s added a commit that referenced this pull request Sep 10, 2026
Top of stack #315, on #322.

Core is moving to an ESM only monorepo
([MetaMask/core#9536](MetaMask/core#9536)), so
this package should arrive already converted rather than landing as the
only hybrid one in it.

## Breaking

`require('@metamask/utils')` now fails with `ERR_REQUIRE_ESM`. `main`
and `module` are gone, and both `.` and `./node` resolve through
`exports` to a single `./dist/*.js` with `./dist/*.d.ts` types.
Consumers already using `import` are unaffected.

| | Before | After |
| --- | --- | --- |
| build | `ts-bridge` | `tsc` |
| output | `.cjs` + `.mjs` + `.d.cts` + `.d.mts` | `.js` + `.d.ts` |
| `@ts-bridge/cli` | yes | removed (`rimraf` added for cleaning) |

`ts-bridge` exists to emit both formats, so it goes with the CJS half.

## Import specifiers

102 relative specifiers across 45 files gained explicit `.js`
extensions, which ESM requires; directories resolve to `/index.js`.
Core's sources already look like this and enforce it with
`n/file-extension-in-import`, so the same three import rules are adopted
here verbatim.

## Two things only the built output revealed

Neither would have been caught by the test suite, because tests run
against `src/`:

- **`lodash`** — `import { memoize } from 'lodash'` throws at runtime
under ESM: Node's lexer cannot see named exports through lodash's CJS.
Switched to `lodash/memoize.js`, a default import of the single method.
Core hit the same wall and solved it with `lodash-es` plus a jest
`moduleNameMapper`; this approach needs neither.
- **`@metamask/scure-bip39`** — the deep wordlist import needed an
explicit `.js`.

Every other CJS dependency survives named imports untouched. `semver`,
`@metamask/superstruct`, `@scure/base`, `@noble/hashes` and `pony-cause`
all have lexer friendly CJS. Verified by importing all 27 built modules
individually, plus the exports map by bare specifier.

## Tooling

`jest.config.js` and `.prettierrc.js` are renamed to `.cjs`, since
`"type": "module"` makes bare `.js` files ESM. Tests still compile to
CommonJS via a ts-jest `transform` override (matching core) with a
`moduleNameMapper` stripping the `.js` specifiers back off.
`constraints.pro` is rewritten for the single entrypoint shape.

<!-- CURSOR_SUMMARY -->
---

> [!NOTE]
> **High Risk**
> Dropping CommonJS and changing the public export map is a breaking
integration change for any consumer still on `require()` or dual-package
resolution, despite unchanged utility APIs.
> 
> **Overview**
> **Breaking:** The package is now **ESM-only** (`"type": "module"`).
`main` / dual `import`/`require` exports are removed; `.` and `./node`
resolve to single `./dist/*.js` + `./dist/*.d.ts`. Plain
`require('@metamask/utils')` fails with `ERR_REQUIRE_ESM` unless
consumers use Node 22+ `require(esm)` or dynamic import (noted in
`CHANGELOG.md`).
> 
> **Build & publish:** Output moves from `ts-bridge` dual
**`.cjs`/`.mjs`** artifacts to **`tsc`-emitted `.js`** (see
`tsconfig.build.json` — declarations are emitted with JS, not
declaration-only). `@ts-bridge/*` is dropped; **`rimraf`** and
`build:clean` handle dist cleanup. Yarn **`constraints.pro`** enforces
the new export shape.
> 
> **Sources:** Relative imports across `src/` use explicit **`.js`**
specifiers (ESLint **`n/file-extension-in-import`** aligned with core).
Runtime fixes for ESM: **`hex.ts`** uses `lodash/memoize.js` (default
import) instead of `{ memoize } from 'lodash'`; **`mnemonic.ts`** adds
`.js` on the deep `@metamask/scure-bip39` wordlist path.
> 
> **Tooling:** Jest stays on **CommonJS** for tests via `ts-jest`
override and a **`moduleNameMapper`** that strips `.js` from relative
paths; config stays **`.cjs`** under `"type": "module"`.
> 
> <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit
7c27d33. Bugbot is set up for automated
code reviews on this repo. Configure
[here](https://www.cursor.com/dashboard/bugbot).</sup>
<!-- /CURSOR_SUMMARY -->
cryptodev-2s added a commit that referenced this pull request Sep 10, 2026
Top of stack #331, on #333. Opts this package into the progressive test
type checking rollout core is doing (16 of its 100 packages so far).

## Tests were not type checked at all

Verified by injecting `const deliberateTypeError: number = "a string"`
into a test file:

| Check | Result |
| --- | --- |
| `yarn build` | passes, `tsconfig.build.json` excludes tests |
| `yarn test:source` | passes, see below |
| `yarn test:types` | passes, tsd only reads `*.test-d.ts` |
| `yarn lint` | passes |

`test:source` can never catch it: `isolatedModules` puts ts-jest on its
`transpileModule` path, which erases types without checking them.

## The change

```
lint:tsc = tsc --noEmit --project tsconfig.lint.json     ← added to `lint`
```

`tsconfig.lint.json` extends `tsconfig.json` (which already covers
everything the build does not) and adds `skipLibCheck`, because
dependency declarations do not satisfy the strict flags used here.

## Why `*.test-d.ts` is excluded

Those belong to `tsd`, which checks them itself via `yarn test:types`
(fixed in #322). Including them here pulls `tsd`'s own types into the
program, and those reach `type-fest`, whose `basic.d.ts` carries `///
<reference lib="esnext" />`. That silently widens built in types past
this package's `lib`, producing a spurious failure where `FrozenSet` no
longer satisfies `ReadonlySet` because of Set methods we do not target.
Traced with `tsc --explainFiles`.

## Why this stacks on ES2022

With `lib: ES2020` a `tsc` pass over the tests reports **11 errors**,
nearly all `Property 'cause' does not exist`. With `lib: ES2022`,
**zero**. #333 has to land first.

## Confirmed not vacuous

A deliberate type error in a test file now exits non-zero with `TS2322`,
and `tsd` still independently catches a bad `expectAssignable` in a
`.test-d.ts` file.

<!-- CURSOR_SUMMARY -->
---

> [!NOTE]
> **Low Risk**
> Dev-only lint and TypeScript config changes; no runtime or shipped
library behavior changes.
> 
> **Overview**
> **`yarn lint` now runs TypeScript on test and other non-build code**
through a new `lint:tsc` script, closing a gap where
`tsconfig.build.json` and transpile-only Jest never validated types in
`*.test.ts` files.
> 
> A new **`tsconfig.lint.json`** extends the root config with
`skipLibCheck`, incremental **`emitDeclarationOnly`** output under
**`.tsc-lint-cache/`** (gitignored and ignored by ESLint), and an
**exclude** for **`*.test-d.ts`** so `tsd`-owned type tests are not
double-checked or polluted by `tsd`’s wider `lib` types.
> 
> <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit
670fa8a. Bugbot is set up for automated
code reviews on this repo. Configure
[here](https://www.cursor.com/dashboard/bugbot).</sup>
<!-- /CURSOR_SUMMARY -->
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.

2 participants