MetaMask / utils

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

Integrate "Are the Types Wrong", and fix issues #162

Open mcmire opened 8 months ago

mcmire commented 8 months ago

Are the Types Wrong? is a tool that scans a library and highlights potential issues with importing it via Node, CommonJS, and ESM.

Running this tool against @metamask/utils shows the following report:

Screenshot 2024-03-05 at 9 58 18 AM

This tool comes with a CLI which we can run manually to see these issues. We can also add it as a lint step in CI to verify that we don't have any issues going forward.

Once we've update this library so that it passes the above checks, we can copy any changes which are necessary to the module template and apply them to every other library.

mcmire commented 6 months ago

@Mrtenz I ran into this tool when researching superstruct. Any thoughts on this?

Mrtenz commented 6 months ago

Interesting. Is there a difference between CJS and ESM type declarations?

I think the fallback conditions will be resolved when we bump to TypeScript >=5.

Mrtenz commented 6 months ago

Node 10 support for the /node export seems irrelevant. We could add a file to the root of the project to support it, but Node 10 has been deprecated for a long time. Any currently active versions support package.json exports.

Mrtenz commented 6 months ago

Just tested with TypeScript 5.3 and it still says "Used fallback condition". Not sure why that happens.

Mrtenz commented 6 months ago

Ah, I just remembered. The order of exports matters. If you move types to the top, it results in:

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

Ah that's interesting!

I realized I didn't answer this question:

Is there a difference between CJS and ESM type declarations?

I think so, yes. I think all imports in ESM type declarations need to end in .js. This could be why it's saying "masquerading as CJS". But I will look into that.

Mrtenz commented 6 months ago

Running tsc with --module es2020 doesn't add extensions to the imports. We could use the dts option of tsup, which would output a d.mts and .d.ts file with appropriate imports. This doesn't work for monorepos though, since tsup doesn't support TypeScript project references.

Mrtenz commented 6 months ago

Gave it a try here: #178. attw fails because the node export doesn't work with <Node16. We may need to run it twice if we want to do it automatically in CI. Once for the main export and package.json, and once for the node export with <Node16 disabled.