mjackson / remix-the-web

Open source tools for Remix (or any framework!)
MIT License
712 stars 14 forks source link

Add main fields to package.json #24

Open petervmeijgaard opened 1 month ago

petervmeijgaard commented 1 month ago

Description

I've been using the @mjackson/headers package in an older project that still uses the old Node module resolution in our tsconfig.json. Because of this, TypeScript will throw an error, indicating that the types are incompatible with the package.

This can also be checked by running the following command:

npx --yes @arethetypeswrong/cli --from-npm @mjackson/headers

Which will show the following output:

┌───────────────────┬──────────────────────────────┬──────────────────────────────────┐
│                   │ "@mjackson/headers"          │ "@mjackson/headers/package.json" │
├───────────────────┼──────────────────────────────┼──────────────────────────────────┤
│ node10            │ 💀 Resolution failed         │ 🟢 (JSON)                        │
├───────────────────┼──────────────────────────────┼──────────────────────────────────┤
│ node16 (from CJS) │ ⚠️ ESM (dynamic import only)  │ 🟢 (JSON)                        │
├───────────────────┼──────────────────────────────┼──────────────────────────────────┤
│ node16 (from ESM) │ 🟢 (ESM)                     │ 🟢 (JSON)                        │
├───────────────────┼──────────────────────────────┼──────────────────────────────────┤
│ bundler           │ 🟢                           │ 🟢 (JSON)                        │
└───────────────────┴──────────────────────────────┴──────────────────────────────────┘

Add the main field to the package.json for each package and the node10 resolution will succeed:

┌───────────────────┬──────────────────────────────┬──────────────────────────────────┐
│                   │ "@mjackson/headers"          │ "@mjackson/headers/package.json" │
├───────────────────┼──────────────────────────────┼──────────────────────────────────┤
│ node10            │ 🟢                           │ 🟢 (JSON)                        │
├───────────────────┼──────────────────────────────┼──────────────────────────────────┤
│ node16 (from CJS) │ ⚠️ ESM (dynamic import only)  │ 🟢 (JSON)                        │
├───────────────────┼──────────────────────────────┼──────────────────────────────────┤
│ node16 (from ESM) │ 🟢 (ESM)                     │ 🟢 (JSON)                        │
├───────────────────┼──────────────────────────────┼──────────────────────────────────┤
│ bundler           │ 🟢                           │ 🟢 (JSON)                        │
└───────────────────┴──────────────────────────────┴──────────────────────────────────┘

I also found out that the lockfile changed when running pnpm install. I've added these changes as well.

How to test

mjackson commented 4 weeks ago

I don't feel super strongly about this (maybe I should) but I'm not explicitly supporting node 10. This is a small change, so if it'd make your life easier I'm ok to merge. But I'm unlikely to merge other types of PR's that are meant for node 10 compat.

Fair?

petervmeijgaard commented 4 weeks ago

Sounds good! I agree that supporting Node 10 is a bit overkill, but as far as I know, the Node 10 column for the @arethetypeswrong/cli util touches versions 10 until 16 of Node.

To give a bit of background, we're still using Next 12 and would really like to use "NodeNext" or "Node16" for "moduleResolution" in our tsconfig.js, but when running next dev, Next overrides these values, forcing us to use the old "Node" resolution, even as we're running Node 20.

Because of this, you'll get the following error message when using @mjackson/headers:

TS2307: Cannot find module "@mjackson/headers" or its corresponding type declarations.

There are types at "/app/node_modules/@mjackson/headers/dist/headers.d.ts", but this
result could not be resolved under your current moduleResolution setting. Consider
updating to "node16", "nodenext", or "bundler"

Matt Pocock also touches on this briefly in his "How To Create An NPM Package" blogpost: https://www.totaltypescript.com/how-to-create-an-npm-package#54-setting-main

petervmeijgaard commented 1 week ago

@mjackson It's been 3 weeks since I opened this merge request. Could you merge this?