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

Very large bundle size #249

Open
achingbrain opened this issue Jan 11, 2024 · 12 comments
Open

Very large bundle size #249

achingbrain opened this issue Jan 11, 2024 · 12 comments
Labels
question Further information is requested

Comments

@achingbrain
Copy link
Contributor

achingbrain commented Jan 11, 2024

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?

achingbrain added a commit to libp2p/js-libp2p that referenced this issue Jan 11, 2024
[rate-limiter-flexible](https://npmjs.com/package/rate-limiter-flexible) is a CJS module with a single export of all it's various implementations.

This defeats tree shaking resulting in the addition of 42KB to the bundle size.

animir/node-rate-limiter-flexible#249

This PR brings the source & tests for the in-memory rate limiter into `@libp2p/utils` which reduces the bundle size increase to a few KBs.
achingbrain added a commit to libp2p/js-libp2p that referenced this issue Jan 11, 2024
[rate-limiter-flexible](https://npmjs.com/package/rate-limiter-flexible) is a CJS module with a single export of all it's various implementations.

This defeats tree shaking resulting in the addition of 42KB to the bundle size.

animir/node-rate-limiter-flexible#249

This PR brings the source & tests for the in-memory rate limiter into `@libp2p/utils` which reduces the bundle size increase to a few KBs.
achingbrain added a commit to libp2p/js-libp2p that referenced this issue Jan 12, 2024
[rate-limiter-flexible](https://npmjs.com/package/rate-limiter-flexible) is a CJS module with a single export of all it's various implementations.

This defeats tree shaking resulting in the addition of 42KB to the bundle size.

animir/node-rate-limiter-flexible#249

This PR brings the source & tests for the in-memory rate limiter into `@libp2p/utils` which reduces the bundle size increase to a few KBs.

---------

Co-authored-by: chad <[email protected]>
@LGmatrix13
Copy link

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

animir commented Jan 17, 2024

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

animir commented Jan 18, 2024

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

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 animir mentioned this issue Jan 21, 2024
@animir
Copy link
Owner

animir commented Jan 21, 2024

@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
Copy link
Contributor

mroderick commented Jan 22, 2024

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

djMax commented Jan 22, 2024

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

@animir animir added the question Further information is requested label Jan 23, 2024
@animir
Copy link
Owner

animir commented Jan 23, 2024

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

@Systerr
Copy link

Systerr commented Feb 29, 2024

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

djMax commented Feb 29, 2024

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

animir commented Feb 29, 2024

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

djMax commented Mar 1, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

6 participants