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

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

Open
mcmire opened this issue Dec 22, 2023 · 8 comments
Open

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

mcmire opened this issue Dec 22, 2023 · 8 comments

Comments

@mcmire
Copy link
Contributor

mcmire commented Dec 22, 2023

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 mcmire changed the title Integrate @arethetypeswrong/cli Integrate "Are the Types Wrong", and fix issues Mar 5, 2024
@mcmire
Copy link
Contributor Author

mcmire commented Mar 5, 2024

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

@Mrtenz
Copy link
Member

Mrtenz commented Mar 5, 2024

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
Copy link
Member

Mrtenz commented Mar 5, 2024

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
Copy link
Member

Mrtenz commented Mar 5, 2024

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

@Mrtenz
Copy link
Member

Mrtenz commented Mar 5, 2024

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
Copy link
Contributor Author

mcmire commented Mar 14, 2024

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
Copy link
Member

Mrtenz commented Mar 15, 2024

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
Copy link
Member

Mrtenz commented Mar 15, 2024

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.

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

No branches or pull requests

2 participants