ai / size-limit

Calculate the real cost to run your JS app or lib to keep good performance. Show error in pull request if the cost exceeds the limit.
MIT License
6.52k stars 1.82k forks source link

support NodeJS ESM Module only project #285

Open unional opened 2 years ago

unional commented 2 years ago

Currently, ESM Module only project doesn't work with size-limit, both esbuild and webpack plugin fails to recognize the project:

// package.json
{
  "type": "module",
  "exports": {
    ".": {
      "import": "./lib/index.js"
    }
  }
}

They both try to look for <proj>/index.js because main was not specified.

ai commented 2 years ago

As quick fix you can specify path to JS file with path option in Size Limit config.

I agree that it will be nice to look for default path in exports too. Can I ask to send PR? I am working on another project right now.

unional commented 2 years ago

Thanks. I take a quick look into the code, but couldn't identify where does it get the default path. :(

btw, is there a esbuild-why similar to webpack-why? (Or if there is a way to get the output file so we can examine it manually)

I have a project that while small, it reports:

✔ Adding to empty esbuild project

  Size limit: 5 kB
  Size:       313 B with all dependencies, minified and gzipped

I'm not sure it is that small~ :)

When I do "path": "./esm/**/*.js" instead, it gives:

✔ Adding to empty esbuild project

  Size limit: 5 kB
  Size:       986 B with all dependencies, minified and gzipped
ai commented 2 years ago

For ESM, you need to specify import with list of exports to test (because of esbuild tree-shaking).

unional commented 2 years ago

For ESM, you need to specify import

Turns out I don't have to. The package is that small. :)

On the other hand, I'm getting these warnings from esbuild when testing the commonJS output in "ECMAScript module`:

Adding to empty esbuild project▲ [WARNING] Top-level "this" will be replaced with undefined since this file is an ECMAScript module

    cjs/index.js:2:23:
      2 │ var __createBinding = (this && this.__createBinding) || (Object.create ? (function(o, m, k, k2) {
        │                        ~~~~
        ╵                        undefined

  This file is considered to be an ECMAScript module because the enclosing "package.json" file sets
  the type of this file to "module":

    package.json:19:10:
      19 │   "type": "module",

[WARNING] The CommonJS "exports" variable is treated as a global variable in an ECMAScript module and may not work as expected

    cjs/index.js:17:0:
      17 │ exports.tersify = void 0;

These output files are used by CommonJS, where the package is configured as follows:

  "type": "module",
  "exports": {
    "types": "./esm/index.d.ts",
    "import": "./esm/index.js",
    "require": "./cjs/index.js"
  },
  "main": "cjs/index.js",

Is there a way to tell esbuild to treat the input as CommonJS?

ai commented 2 years ago

It should detects it automatically by package.type

unional commented 2 years ago

It should detects it automatically by package.type

It does, which is the problem here.

because under type: module, you can export both ESM and CommonJS (as seen in the example above).

As for size-limit usage, I'm checking both:

"size-limit": [
  { "path": "./cjs/index.js", "limit": "5kB" },
  { "path": "./esm/index.js", "limit": "5kB" }
]
ai commented 2 years ago

You need to explicitly add .cjs extension for CJS file (or add package.json to ./cjs/).

Otherwise, your npm package will not work in Node.js.

unional commented 2 years ago

TypeScript (including the latest 4.7) does not support that.

ai commented 2 years ago

Welcome to the hell of ESM migration :D. I can’t help with TS.

unional commented 2 years ago

🤣 no problem. ESM migration with TypeScript is hell x 2.

Here are some links related to it if you are interested:

https://github.com/microsoft/TypeScript/issues/49083 https://github.com/microsoft/TypeScript/issues/49271 https://github.com/microsoft/TypeScript/issues/49335

antongolub commented 2 years ago

ESM migration with TypeScript and Jest is hell^3.

@unional, if understand correctly, it seems that's necessary to inject .js extensions into .ts/.d.ts but rename target/cjs/*.js to.cjs.

unional commented 2 years ago

@unional, if understand correctly, it seems that's necessary to inject .js extensions into .ts/.d.ts but rename target/cjs/*.js to.cjs.

What I understand is to these two things:

I'm still not sure about changing the .d.ts and cjs/*.js post transpilation. Because that could break sourceMap, inlineSourceMap, and inlineSources.

I think that's also part of the reason TypeScript team don't want to do it.

JounQin commented 2 years ago

We can use import() instead of require() easily to support ESM only packages AFAIK, I'd like to try to raise a PR for it.

JounQin commented 2 years ago

@unional

I'm still not sure about changing the .d.ts and cjs/*.js post transpilation.

See https://github.com/sheremet-va/dual-packaging

You need something like rollup