Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use ts-bridge for building #182

Merged
merged 2 commits into from
Apr 23, 2024
Merged

Use ts-bridge for building #182

merged 2 commits into from
Apr 23, 2024

Conversation

Mrtenz
Copy link
Member

@Mrtenz Mrtenz commented Apr 18, 2024

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.

Copy link

socket-security bot commented Apr 18, 2024

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↗︎

@Mrtenz Mrtenz marked this pull request as ready for review April 22, 2024 10:17
@Mrtenz Mrtenz requested a review from a team as a code owner April 22, 2024 10:17
@Mrtenz Mrtenz requested a review from mcmire April 22, 2024 13:30
@mcmire
Copy link
Contributor

mcmire commented Apr 22, 2024

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:

  • create-release-branch (which is ESM) => works without a hitch! ✅
  • core (which is CommonJS) => works! ✅ (with the exception of a change I had to make to a Jest config, but that's on us)

Seems good to me :)

@Mrtenz
Copy link
Member Author

Mrtenz commented Apr 23, 2024

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.

@Mrtenz Mrtenz merged commit 1c408dd into main Apr 23, 2024
19 checks passed
@Mrtenz Mrtenz deleted the mrtenz/ts-bridge branch April 23, 2024 11:08
MajorLift added a commit to MetaMask/superstruct that referenced this pull request May 29, 2024
)

## Explanation

As part of our OKR for upgrading TypeScript to v5.0+ (Q2 2024 O3KR4), we
are updating our core dependencies to generate builds and type
declarations for both CJS and ESM.

## References

- Same update made to `@metamask/utils`:
MetaMask/utils#182
- Required to update core repo's `moduleResolution` setting to
`NodeNext`:
  - Blocks: MetaMask/core#3651
 
## Changelog

### Added

- Newly exports the following types: `AnyStruct`, `EnumSchema`,
`InferStructTuple`, `IsExactMatch`, `IsMatch`, `IsRecord`, `IsTuple`,
`ObjectSchema`, `OmitBy`, `Optionalize`, `PickBy`, `Simplify`,
`UnionToIntersection`.
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.

None yet

3 participants