feat!: replace hybrid CJS/ESM build with ESM-only - #9536
Conversation
Mrtenz
changed the title
4a98147 to
db128a5
Compare
|
All alerts resolved. Learn more about Socket for GitHub. This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. |
d99b699 to
2438c87
Compare
91b5c8e to
fa18841
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
I've assigned the |
@Gudahtt This PR targets |
| "scripts": { | ||
| "build": "ts-bridge --project tsconfig.build.json --verbose --clean --no-references", | ||
| "build:all": "ts-bridge --project tsconfig.build.json --verbose --clean", | ||
| "build": "tsc --project tsconfig.build.json", |
There was a problem hiding this comment.
Previously yarn build would automatically clean dist. With these changes it appears that is no longer the case. Is it worth adding a build:clean script to all packages in case that is needed? Or should we drop it for now?
There was a problem hiding this comment.
Good point, we can add a build:clean script to all packages. If we make it clean by default we don't get the benefits of incremental builds.
| // TS2742: The inferred type of 'selectAllAssets' cannot be named without a | ||
| // reference to '../../../../node_modules/@metamask/account-tree-controller/dist/wallet.js'. | ||
| // This is likely not portable. A type annotation is necessary. | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| export const selectAllAssets: any = createAssetListSelector( |
There was a problem hiding this comment.
Yeah that's definitely interesting...
| "lint:tsconfigs:fix": "tsx ../../scripts/lint-tsconfigs/lint-tsconfigs.mts --fix", | ||
| "messenger-action-types:check": "tsx ../../packages/messenger-cli/src/cli.ts --esm --check", | ||
| "messenger-action-types:generate": "tsx ../../packages/messenger-cli/src/cli.ts --esm --generate", | ||
| "messenger-action-types:check": "tsx ../../packages/messenger-cli/src/cli.ts --formatter oxfmt --esm --check", |
There was a problem hiding this comment.
Are these scripts not enforced via yarn.config.cjs?
| "url": "https://github.com/MetaMask/core.git" | ||
| }, | ||
| "bin": "./dist/bin/java-tron-up.mjs", | ||
| "bin": "./dist/bin/java-tron-up.js", |
There was a problem hiding this comment.
Interesting, is this required?
Never mind, I understand why this change is needed now.
| try { | ||
| wallet = importers.fromEtherWallet(input, password); | ||
| } catch { | ||
| // @ts-expect-error: Wallet.fromV3 does not exist? |
There was a problem hiding this comment.
That's weird. I guess this line still works at runtime?
There was a problem hiding this comment.
It does, not sure why the types are broken. 😕
| ...updatedSmartTransaction, | ||
| }; | ||
|
|
||
| if (updatedSmartTransaction.status === prevSmartTransaction?.status) { |
There was a problem hiding this comment.
Feels like there ought to be a better way to do this than deep cloning. Oh well...
In any case, why not use cloneDeep? Other packages seem to be using this.
There was a problem hiding this comment.
Replaced this with klona but then found many other instances of lodash after so ended up replacing that with lodash-es. Will do the same here.
| jest.mock('./utils'); | ||
| import { jest } from '@jest/globals'; | ||
|
|
||
| // `jest.mock` does not apply to ES modules, so the module registry is stubbed |
There was a problem hiding this comment.
Oof. Seems strange that we have to dynamically import the modules.
Is there something we're doing wrong here? Or is this one of those instances where Vitest provides a much better experience?
There was a problem hiding this comment.
This is just how Jest works with ESM unfortunately, and why I've been pushing back on the full Jest to ESM migration. With Vitest we shouldn't need to dynamically import anything.
| '<rootDir>/../../node_modules/@metamask/$1', | ||
| ], | ||
| '^uuid$': require.resolve('uuid'), | ||
| // The sources import `lodash-es`, because `lodash` is CommonJS and its |
There was a problem hiding this comment.
Good work figuring this out 👏🏻
|
|
||
| preset: 'ts-jest', | ||
|
|
||
| // Unlike the packages, the scripts use `import.meta`, which has no CommonJS |
There was a problem hiding this comment.
Interesting. I wonder if we can clean this up in the future so we don't have to make these overrides. Or maybe this is just how it has to work from now on until we switch to Vitest...
There was a problem hiding this comment.
I think we should just start migrating to Vitest at some point. Cleaning this up otherwise would likely mean migrating all Jest tests to ESM.
| "build:clean": "yarn build:only-clean && yarn build", | ||
| "build:docs": "yarn workspaces foreach --all --no-private --parallel --interlaced --verbose run build:docs", | ||
| "build:only-clean": "rimraf -g 'packages/*/dist'", | ||
| "build:only-clean": "rimraf -g 'packages/*/dist' 'packages/*/tsconfig.build.tsbuildinfo'", |
There was a problem hiding this comment.
Thank you, I had always meant to add this but never did 🤦🏻
There was a problem hiding this comment.
It would be useless anyway for ts-bridge 😅.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit fe8f4f8. Configure here.
Switch from `@ts-bridge/cli` to standard `tsc` with TypeScript project references for building all packages.
Remove CJS entry points (`main`, `types`, `require`) from all package manifests and simplify `exports` to a single `import` condition. This drops the dual CJS/ESM output in favour of ESM-only distribution.
Mark all packages (and the root) as native ESM by adding `"type": "module"` to their manifests. Also adds a Yarn constraint to enforce this field going forward.
`__dirname` is not available in ESM. Replace all usages with `import.meta.dirname`, which is equivalent and available since Node.js 21.2.
Older versions of TypeScript break when using named imports in ESM. Import the default export and destructure instead as a temporary workaround. Also passes `--formatter oxfmt` to the `messenger-action-types` scripts in packages missing it.
Adding `"type": "module"` to every package made each `jest.config.js` an ES module, so the `require` and `module.exports` they are written with no longer resolved and Jest could not start at all. Rather than convert the test setup to ESM, the configs move to `.cjs` and Jest keeps running as CommonJS. A `ts-jest` transform override compiles the sources to CommonJS for tests only, which leaves the published build untouched. This works because Jest treats TypeScript as ESM only when the extension is listed in `extensionsToTreatAsEsm`, which it is not here. Keeping the tests on CommonJS avoids importing `jest` from `@jest/globals` in every test file, swapping `lodash` for `lodash-es`, and rewriting `jest.mock` calls as `jest.unstable_mockModule`. It also resolves the failures where an external CommonJS dependency could not `require()` a monorepo package that had become ESM-only. The trade-off is that the suite exercises CommonJS while the packages ship ESM, so genuinely ESM-only problems are caught by the build and type checks rather than by the tests.
`"type": "module"` in the root manifest makes every `.js` file an ES module, so `.prettierrc.js` could no longer use `module.exports`. It moves to `.prettierrc.cjs`, and the entry point in `knip.config.mts` is updated to match. The `permission-controller` ESLint config is removed rather than renamed. It extends a root `.eslintrc.js` that no longer exists now that the repo uses flat config in `eslint.config.mjs`, and nothing references it, so it has no effect.
The packages compile to CommonJS for tests, but the scripts cannot: they use `import.meta`, which has no CommonJS equivalent. Their Jest config opts into ESM instead, through `extensionsToTreatAsEsm` and the `useESM` option of `ts-jest`. Running as ESM means `require` and the injected `jest` global are gone, so `utils.ts` loads the Prettier config through `createRequire`, and the test files import `jest` from `@jest/globals`. `jest.mock` has no effect on ES modules, so the mocked modules move to `jest.unstable_mockModule` and are pulled in with dynamic imports afterwards. The `process.exit` spy in `cli.test.ts` now has to be installed before `cli.js` is imported. The ESM build of yargs captures `process.exit` by reference when it loads, so a spy installed later is never consulted, and its implementation is re-armed per test because `resetMocks` clears it.
`lodash` is CommonJS, so its named exports cannot be imported from an ES
module: `import { cloneDeep } from 'lodash'` throws at run time once the
packages are loaded as ESM. This went unnoticed because the tests compile
to CommonJS, where the same import is fine. It surfaced in the
`wallet-cli` daemon end-to-end test, which spawns the built CLI and so
loads the real ES modules.
The sources now import `lodash-es`, which publishes the same API as an ES
module. Jest maps it back to `lodash` for the tests, since the reverse
problem applies there: an ES module cannot be `require`d.
Two call sites needed more than a rename. `smart-transactions-controller`
imported the default export, which `lodash-es` does not provide, so it
imports the functions it uses by name. `assets-controller` referred to the
`MemoizedFunction` type, which `@types/lodash-es` does not re-export, so
it takes that type from `@types/lodash` instead; the import is erased at
compile time and adds no run-time dependency.
`ethereumjs-wallet` is CommonJS, and Node cannot reliably detect its named
exports, so `import { thirdparty }` throws once the package is loaded as an
ES module. Node 22 fails on it where Node 24 happens to cope, which is why
this only appeared in the `wallet-cli` daemon end-to-end test.
The package also declares a TypeScript-style default export, so
`module.exports` is reached through `default` under Node's interop but is
the namespace itself once `esModuleInterop` has unwrapped it for the
CommonJS test build. Both shapes are now resolved explicitly, which also
fixes `Wallet.fromV3` being `undefined` under ESM.
Support for Node 18 and 20 was dropped in #9168, which set `engines` to `^22.14.0 || ^24` and trimmed the test matrices, but the `wallet-cli` daemon end-to-end job kept its own matrix and was missed. Node 20 also lacks the ESM interop these packages now rely on.
The build info files record which outputs `tsc` has already produced, so deleting `dist` without them leaves the two disagreeing. Incremental builds then skip work whose output is gone, which surfaces as a cascade of TS6305 "has not been built from source file" errors that a clean build does not resolve.
Under `moduleResolution: "Node16"`, TypeScript decides whether a declaration file is ESM or CommonJS from the nearest `package.json` `"type"` field. `@metamask/safe-event-emitter` declares no `"type"`, so its ESM declarations were read as CommonJS and the default import resolved to the module namespace rather than the class, leaving `class JsonRpcEngine extends SafeEventEmitter` failing to build. That was carried as a patch adding an `index.d.mts`, which TypeScript always treats as ESM. Version 3.1.3 ships the same fix, so the patch and its resolutions are removed in favour of the released version. Pinning to a patched `3.1.1` would also have silently dropped the fix as soon as anything asked for a newer version.
3 participants
Explanation
Currently, packages in this monorepo are built as both CommonJS and ESM using
@ts-bridge/cli. This PR replaces that with ESM-only output built with standardtscand TypeScript project references.The motivation is to simplify the build toolchain by removing the
ts-bridgedependency in preparation for the TypeScript 6+ upgrade.References
Checklist
Note
High Risk
Breaking NPM surface (ESM-only exports) affects every downstream consumer still on
require(), plus a monorepo-wide build and test pipeline change with high blast radius.Overview
Breaking: Published packages move from dual CommonJS/ESM (
ts-bridge→dist/index.cjs/.mjs) to ESM-only output viatsc --build(dist/index.js+index.d.ts), with"type": "module"on the root and every workspace package.@ts-bridge/cliis removed; CI partial builds andbuild:only-cleannow usetscand also wipetsconfig.build.tsbuildinfo.Tooling is realigned for that model: Jest keeps package tests on CommonJS compilation (
ts-jest+lodash-es→lodashmapper) while script tests run as ESM (extensionsToTreatAsEsm); shared configs move to*.cjs(includingjest.environment.cjs). ESLint/knip follow the same split (.cjsvs.jsmodule semantics, Jestjestshadow allowance).Source fixes support ESM resolution: widespread
lodash→lodash-es, explicit.jsimport suffixes, namedbignumber.jsimports, and small type/workarounds (e.g.uuidV4Options,selectAllAssetsannotation). Docs drop the ts-bridge mention; wallet-cli e2e CI drops Node 20.x from the matrix.Reviewed by Cursor Bugbot for commit b9c9656. Bugbot is set up for automated code reviews on this repo. Configure here.