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

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

Closed
sachinraja opened this issue Jul 8, 2022 · 5 comments
Closed

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

sachinraja opened this issue Jul 8, 2022 · 5 comments

Comments

@sachinraja
Copy link

sachinraja commented Jul 8, 2022

In the new v2.3.0-beta build, outExtension was overridden with the build format:

outExtension({ format }) {
return {
js: format === 'cjs' ? '.js' : `.${format}.js`,
};
},

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.

@sachinraja sachinraja changed the title build is not esm compliant v2.3.0-beta build is not esm compliant Jul 8, 2022
@timolins
Copy link
Owner

timolins commented Jul 9, 2022

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

sachinraja commented Jul 9, 2022

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

timolins commented Jul 9, 2022

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

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.

timolins added a commit that referenced this issue Jul 9, 2022
* Update package.json according to #204
@sachinraja
Copy link
Author

Thanks @timolins! The new release looks good

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