MetaMask / metamask-module-template

A simple template repository for starting new modules in the latest MetaMask fashion.
26 stars 23 forks source link

Build package as both CJS and ESM #203

Closed Mrtenz closed 1 year ago

Mrtenz commented 1 year ago

Instead of transpiling the source code to just CJS, we now transpile it to ESM as well. This is beneficial for projects using bundlers that are capable of tree-shaking (such as MetaMask Snaps), because it's much easier to determine whether the code has side effects or not.

For the transpilation, we now use SWC rather than TSC. This avoids having to do type checking twice, which is quite slow. TypeScript is still used for generating the declaration files, however, so the code is still being type-checked.

This is based on a change in the Snaps repository (MetaMask/snaps#1519), which has been tested in the extension, so this shouldn't cause any problems there.

socket-security[bot] commented 1 year ago

👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎

This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored.

Ignoring: source-map@0.7.4, clone-response@1.0.3, responselike@2.0.1, @mole-inc/bin-wrapper@8.0.1, @swc/cli@0.1.62, @swc/core@1.3.68, arch@2.2.0, got@11.8.6, readable-web-to-node-stream@3.0.2, strip-outer@2.0.0, strip-eof@1.0.0, pump@3.0.0, @tokenizer/token@0.3.0, executable@4.1.1, bin-check@4.1.0, semver-truncate@3.0.0, sort-keys@1.1.2, lowercase-keys@2.0.0, find-versions@5.1.0, filename-reserved-regex@3.0.0, defer-to-connect@2.0.1, content-disposition@0.5.4, json-buffer@3.0.1, bin-version@6.0.0, @szmarczak/http-timer@4.0.6, @types/http-cache-semantics@4.0.1, mime-db@1.52.0, pseudomap@1.0.2, end-of-stream@1.4.4, semver-regex@4.0.5, ext-list@2.2.2, ieee754@1.2.1, quick-lru@5.1.1, os-filter-obj@2.0.0, sort-keys-length@1.0.1, p-finally@1.0.0, trim-repeated@2.0.0, ext-name@5.0.0, mimic-response@1.0.1, mimic-response@3.1.0, resolve-alpn@1.2.1, http2-wrapper@1.0.3, cacheable-request@7.0.4, cacheable-lookup@5.0.4

Next steps

Take a deeper look at the dependency

Take a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev.

Remove the package

If you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency.

Mark a package as acceptable risk

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of package-name@version specifiers. e.g. @SocketSecurity ignore foo@1.0.0 bar@* or ignore all packages with @SocketSecurity ignore-all

socket-security[bot] commented 1 year ago

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

Packages Version New capabilities Transitives Size Publisher
@swc/cli 0.1.62 network, filesystem, shell, environment +83 386 MB kdy1
@swc/core 1.3.68 filesystem, shell, environment +10 384 MB kdy1
Mrtenz commented 1 year ago

After some more testing, I found that Browserify does not support the exports field (browserify/resolve#222). This means that we can't use any scoped imports, as they will not work in the extension. For example, if @metamask/foo contains the following import:

import { bar } from '@metamask/bar/baz'; // dist/cjs/baz/index.js

This will work fine in Node.js, Webpack, etc., but in Browserify this results in an error:

Could not resolve "@metamask/bar/baz"

I have an idea on how this can be solved, but will put it into draft until this has been tested.

Gudahtt commented 1 year ago

Could you elaborate on the connection between that problem, and the contents of this PR? I see that this PR adds the exports field, but it's less clear to me how this impacts browserify (i.e. does it matter that browserify can still only use the main/module entrypoint?)

Mrtenz commented 1 year ago

Could you elaborate on the connection between that problem, and the contents of this PR? I see that this PR adds the exports field, but it's less clear to me how this impacts browserify (i.e. does it matter that browserify can still only use the main/module entrypoint?)

Sure. The problem is that packages that don't use Browserify could import in ways that are not supported by Browserify. For example, if @metamask/utils imports from @metamask/key-tree/some/path, and @metamask/utils gets imported into the extension, Browserify would fail because node_modules/@metamask/key-tree/some/path does not exist. It doesn't look at the exports field to try and resolve the proper path, i.e., node_modules/@metamask/key-tree/dist/cjs/some/path.

We solved this in the Snaps repo by explicitly exporting ./dist/cjs/* and ./dist/esm/*, rather than ./*. This means that @metamask/utils can import @metamask/key-tree/dist/cjs/some/path, because it's specified in the exports field, and Browserify can import it, because it's an actual path on the disk: node_modules/@metamask/key-tree/dist/cjs/some/path exists.

Ideally we avoid importing from package/dist in the first place, but since we're doing that, I think this is the best solution as long as we're still using Browserify.

I hope that makes sense!

Gudahtt commented 1 year ago

Thanks! Makes sense.

Perhaps in the meantime we could use the main and module fields to support multiple build types? Reserving the nicer export entry points for later, when that problem is resolved.

Mrtenz commented 1 year ago

Thanks! Makes sense.

Perhaps in the meantime we could use the main and module fields to support multiple build types? Reserving the nicer export entry points for later, when that problem is resolved.

I hadn't even considered that we don't need exports at all in that case. 😅

Gudahtt commented 1 year ago

@SocketSecurity ignore @swc/cli@0.1.62 @SocketSecurity ignore @swc/core@1.3.68

These packages are expected to have filesystem and shell access. And core needs a postinstall script to prepare native code.

Gudahtt commented 1 year ago

@SocketSecurity ignore-all

The remaining alerts are regarding packages used by SWC with access that seems reasonable for them to have, and new author/unmaintained warnings (none of which seem suspicious).