animir / node-rate-limiter-flexible

Atomic counters and rate limiting tools. Limit resource access at any scale.
ISC License
3.04k stars 157 forks source link

Very large bundle size #249

Open achingbrain opened 8 months ago

achingbrain commented 8 months ago

When using the in-memory rate limiter in browsers it results in a bundle addition of 42KB:

image

If I pull just the depended on files out of this module it's more like 4KB:

image

I think this is because this module is CJS and exports all the various implementations in one blob which makes tree shaking impossible.

Could this module be updated to be ESM with only named exports?

LGmatrix13 commented 8 months ago

Not sure how much of a drop-in replacement this would be, but bun has become a popular bundler lately since it can export a package to all the different js extensions, including ESM.

https://bun.sh/docs/bundler#content-types

animir commented 8 months ago

@achingbrain The question about ESM is interesting. I don't have much experience with tree-shaking optimizations. Different projects use different approaches. Can we keep both CJS and ESM without breaking dependent projects? Do you have any suggestions on changing it with or without breaking changes?

animir commented 8 months ago

@LGmatrix13 Thanks, it looks promising. I still get my head around the idea of having ESM module exports. Not sure, what's the best practice here. Should it be provided with both options CJS and ESM? Or just ESM and deprecate using CJS? If both how it is published to npm? Maybe I asked you too many questions already :-) Sorry, just not sure how to proceed.

achingbrain commented 8 months ago

Can we keep both CJS and ESM without breaking dependent projects?

There are a few caveats around shipping both - the same authored class imported by a CJS file and a ESM file is treated as a separate class by the js runtime (since the classes were loaded from different locations) so were you to pass the instance between them instanceof won't work as expected, and like this you can end up including both versions in your bundle by accident.

Tools like rollup will create CJS/ESM versions and you can let the runtime select them with an "exports" entry in the project's package.json:

{
  "exports": {
    ".": {
      "types": "./path/to/index.d.ts",
      "require": "./path/to/index.cjs.js",
      "import": "./path/to/index.esm.js"
    }
  }
}

From my experience the short term pain of going ESM-only is preferable to the long-term pain of supporting both CJS and ESM exports.

animir commented 8 months ago

@achingbrain Thank you for your valuable input.

I think, it is better to go clean ESM since Node.js natively supports it. I've prepared a PR with refactoring. Could you double check this index.js https://github.com/animir/node-rate-limiter-flexible/pull/250/files#diff-e727e4bdf3657fd1d798edcd6b099d6e092f8573cba266154583a746bba0f346 is what allows to make optimal tree-shaking?

mroderick commented 8 months ago

This issue is really a shortcoming of the bundler/treeshaker used by OP and not of rate-limiter-simple.

Since OP is using ESM, they can deep import files from the package, circumventing ~to~ the root file and helping their treeshaker out.

import RateLimiterMemory from "rate-limiter-flexible/lib/RateLimiterMemory.js";

console.log(typeof RateLimiterMemory);
// => function

That means that we don't need to convert this codebase to ESM just yet or in a hurry, but can take our time and do it without introducing new problems for users.

djMax commented 8 months ago

the words "clean" and "ESM" are antonyms. Please don't convert this repo.

animir commented 8 months ago

@djMax Could you tell more about your concern on ESM?

Systerr commented 7 months ago

Converting to ESM is a really good idea. Node.js has natively supported it for a long time, and browsers also support it. For CJS (CommonJS) node executions, developers can use "async import()" (dynamic import)

djMax commented 7 months ago

I think the only sane solution here is dual output packages - CJS and ESM. ESM causes no end of pain with complex environments - jest, webpack, nextjs, they all have their own solutions to this pain and they often fight each other. As a library developer, the nice thing to do is output both and let the particular use case choose based on the environment.

https://www.sensedeep.com/blog/posts/2021/how-to-create-single-source-npm-module.html

animir commented 7 months ago

@djMax Hi, thank you for sharing the link. Since ESM is now natively supported by Node.js should we all use ESM and abandon CJS?

djMax commented 7 months ago

No, because it is just not that simple. Node is one environment, but webpack, jest, vitest, typescript - they are all different and all have varying levels of support. And ESM is like a poison that once it puts a drop in your bucket, no other package can do anything but ESM. Whereas CJS works just fine in either ESM or CJS. So dual bundles are require to get flexibility on the backend and tree shaking on the front end.