MetaMask / utils

Various JavaScript / TypeScript utilities of wide relevance to the MetaMask codebase.
https://metamask.github.io/utils/index.html
ISC License
31 stars 10 forks source link

Use `ts-bridge` for building #182

Closed Mrtenz closed 6 months ago

Mrtenz commented 6 months ago

Turns out tsup wasn't the perfect solution either (mainly due to problems with ESBuild), so we decided to switch to a different solution, hopefully for the last time. ts-bridge also simplifies the build process a little bit, as we no longer need a separate config.

Output from attw:

┌───────────────────┬───────────────────┬────────────────────────┬────────────────────────────────┐
│                   │ "@metamask/utils" │ "@metamask/utils/node" │ "@metamask/utils/package.json" │
├───────────────────┼───────────────────┼────────────────────────┼────────────────────────────────┤
│ node10            │ 🟢                │ 💀 Resolution failed   │ 🟢 (JSON)                      │
├───────────────────┼───────────────────┼────────────────────────┼────────────────────────────────┤
│ node16 (from CJS) │ 🟢 (CJS)          │ 🟢 (CJS)               │ 🟢 (JSON)                      │
├───────────────────┼───────────────────┼────────────────────────┼────────────────────────────────┤
│ node16 (from ESM) │ 🟢 (ESM)          │ 🟢 (ESM)               │ 🟢 (JSON)                      │
├───────────────────┼───────────────────┼────────────────────────┼────────────────────────────────┤
│ bundler           │ 🟢                │ 🟢                     │ 🟢 (JSON)                      │
└───────────────────┴───────────────────┴────────────────────────┴────────────────────────────────┘

@metamask/utils/node is failing because Node.js 10 doesn't support package.json exports. Node 16 (EOL) and higher support this, so that's a non-issue.

socket-security[bot] commented 6 months ago

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package New capabilities Transitives Size Publisher
npm/@ts-bridge/cli@0.1.1 Transitive: environment, filesystem +1 510 kB mrten
npm/@ts-bridge/shims@0.1.1 None 0 19.8 kB mrten
npm/@types/node@20.12.7 None 0 2.03 MB types
npm/chalk@5.3.0 None 0 43.7 kB sindresorhus
npm/jackspeak@2.2.1 environment, unsafe 0 222 kB isaacs
npm/path-scurry@1.10.1 filesystem +2 1.27 MB isaacs
npm/undici-types@5.26.5 None 0 73.1 kB ethan_arrowood

🚮 Removed packages: npm/@esbuild/android-arm64@0.19.12, npm/@esbuild/android-arm@0.19.12, npm/@esbuild/android-x64@0.19.12, npm/@esbuild/darwin-arm64@0.19.12, npm/@esbuild/darwin-x64@0.19.12, npm/@esbuild/freebsd-arm64@0.19.12, npm/@esbuild/freebsd-x64@0.19.12, npm/@esbuild/linux-arm64@0.19.12, npm/@esbuild/linux-arm@0.19.12, npm/@esbuild/linux-ia32@0.19.12, npm/@esbuild/linux-loong64@0.19.12, npm/@esbuild/linux-mips64el@0.19.12, npm/@esbuild/linux-ppc64@0.19.12, npm/@esbuild/linux-riscv64@0.19.12, npm/@esbuild/linux-s390x@0.19.12, npm/@esbuild/linux-x64@0.19.12, npm/@esbuild/netbsd-x64@0.19.12, npm/@esbuild/openbsd-x64@0.19.12, npm/@esbuild/sunos-x64@0.19.12, npm/@esbuild/win32-arm64@0.19.12, npm/@esbuild/win32-ia32@0.19.12, npm/@esbuild/win32-x64@0.19.12, npm/@types/node@17.0.45, npm/any-promise@1.3.0, npm/binary-extensions@2.3.0, npm/bundle-require@4.0.2, npm/cac@6.7.14, npm/chokidar@3.6.0, npm/esbuild@0.19.12, npm/is-binary-path@2.1.0, npm/jackspeak@2.3.6, npm/joycon@3.1.1, npm/lilconfig@3.1.1, npm/load-tsconfig@0.2.5, npm/lodash.sortby@4.7.0, npm/mz@2.7.0, npm/object-assign@4.1.1, npm/path-scurry@1.10.2, npm/postcss-load-config@4.0.2, npm/rollup@4.16.1, npm/sucrase@3.35.0, npm/thenify-all@1.6.0, npm/thenify@3.3.1, npm/tr46@1.0.1, npm/tree-kill@1.2.2, npm/ts-interface-checker@0.1.13, npm/tsup@7.3.0, npm/webidl-conversions@4.0.2, npm/whatwg-url@7.1.0

View full report↗︎

mcmire commented 6 months ago

I'm noticing that the requires in the CommonJS version of files have extensions in them, even though they aren't necessary. I don't think this is an issue but something to watch for.

In any case I ran yarn build on branch and then linked utils into these projects:

Seems good to me :)

Mrtenz commented 6 months ago

I'm noticing that the requires in the CommonJS version of files have extensions in them, even though they aren't necessary. I don't think this is an issue but something to watch for.

This is intentional. The ESM module resolution algorithm is quite complicated, but if a file is using .mjs or .cjs, it must be an ES module or CommonJS respectively.

So in theory this lets Node.js (or whatever module resolution algorithm) take the happy path, and continue parsing the file using the module type implied by the file extension, and skip looking at other files that can determine if something is ESM or CJS.

It also removes the need for checking different file extensions. Node.js might look for file.js before file.cjs for example.

So theoretically it's slightly more performant to use file extensions everywhere. I doubt it makes any significant difference in practice, but as far as I know there's no downsides to using file extensions everywhere either.