timolins / react-hot-toast

Smoking Hot React Notifications 🔥
https://react-hot-toast.com
MIT License
9.66k stars 319 forks source link

v2.3.0-beta build is not esm compliant #204

Closed sachinraja closed 2 years ago

sachinraja commented 2 years ago

In the new v2.3.0-beta build, outExtension was overridden with the build format: https://github.com/timolins/react-hot-toast/blob/39c1bdac9ff3d8e17c2e65288717cf4fcb19a501/tsup.config.ts#L9-L13

This is not compliant with esm. tsup emits with the .mjs extension by default because packages with "type": "commonjs" (all packages implicitly have this) must use the .mjs extension for esm files according to Node's resolution. Bundlers may be more forgiving.

timolins commented 2 years ago

Interesting, thanks for pointing this out.

I introduced this overwrite because size-limit had problems importing the .mjs file. This no longer seems to be a problem after updating to the latest version, which uses esbuild instead of webpack.

Nonetheless, it's somewhat concerning that some tools can't handle .mjs by default.

I found this article which also points to this problem.

Can you point me to an official resource that suggest how it should be done? I'd love to do this in an ESM compliant way, but without compromising support for commonly used build setups.


Preact seems to export both (.esm.js & .mjs).

sachinraja commented 2 years ago

This no longer seems to be a problem after updating to the latest version, which uses esbuild instead of webpack.

I really don't like these tools for that reason. To really get an overview of the size, you need to test with multiple bundlers.

I found this article which also points to this problem.

That article doesn't suggest a good practice btw.

Just to provide some qualification here, I added ESM support to react-query.

Nonetheless, it's somewhat concerning that some tools can't handle .mjs by default.

I don't try to support these older tools. Instead, I just force them to use cjs.

Preact seems to export both (.esm.js & .mjs).

Preact has a bigger interest here since they want to have the smallest size possible, even for older bundlers.

Bundlers that support "exports" typically also support esm/.mjs. The tradeoff in implementing my solution here is that older bundlers/bundler versions will only get commonjs so there won't be any tree shaking. Mind if I do a PR to show you what I mean?

timolins commented 2 years ago

Thanks for your insights, interesting points.

I really don't like these tools for that reason. To really get an overview of the size, you need to test with multiple bundlers.

You are right, they definitely don't tell you the whole story. I still like to use them, because they give me a feel what impact a PR has on bundle size. (Did it add or remove weight?)

I don't try to support these older tools. Instead, I just force them to use cjs.

Good point that old bundlers can use cjs as fallback. The problem with my size-limit example was, that I forced .mjs on it, which wouldn't happen if the bundler resolved it by itself.

Mind if I do a PR to show you what I mean?

That'd be cool. I'm playing around with it as well. This is a snippet of the version I have right now. Is that what you envisioned?

  "main": "dist/index.js",
  "module": "dist/index.mjs",
  "types": "dist/index.d.ts",
  "exports": {
    "./package.json": "./package.json",
    ".": {
      "import": "./dist/index.mjs",
      "require": "./dist/index.js",
      "types": "./dist/index.d.ts"
    },
    "./headless": {
      "import": "./headless/index.mjs",
      "require": "./headless/index.js",
      "types": "./headless/index.d.ts"
    }
  }
sachinraja commented 2 years ago

Yes that's mostly right! But if you're using the "types" field in "exports" it needs to come first (see TS docs). Also the top level "module" field should be removed, it's a nonstandard one so it could break with the mjs extension.

sachinraja commented 2 years ago

Thanks @timolins! The new release looks good