Eliav2 / react-xarrows

Draw arrows (or lines) between components in React!
https://codesandbox.io/embed/github/Eliav2/react-xarrows/tree/master/examples?fontsize=14&hidenavigation=1&theme=dark
MIT License
584 stars 75 forks source link

Entirety of Lodash is Bundled (over 75% of package size) #118

Open joealden opened 2 years ago

joealden commented 2 years ago

Describe the bug & Expected behavior

As shown here on Bundlephobia, the current version of the package results in all of lodash being bundled with it. The "Composition" section on Bundlephobia shows that this makes up 76.4% of the package size (I can also verify that this is true as when I bundle this via webpack in my app, it adds all of lodash to my bundle).

From a quick look at the codebase, lodash is only being imported in GetPosition.tsx and useXarrowProps.ts, and in those files, only pick and isEqual are actually being used.

Some notes on potential solutions to this:

  1. The current pick usage could likely be replaced by simpler JS (for example: { x: chosenStart.x, y: chosenStart.y }).
  2. The lodash dependency could be replaced by lodash-es, and this should improve tree shaking. The caveat with this is that if we were to keep the dependencies as being treated as "external" via webpack, this could be a breaking change (as I believe it'd directly expose ESM to users). With that being said, if we were to bundle only the required utils directly in the bundle itself (so moving lodash to be a dev dependency), this likely wouldn't increase the bundle size very much (as shown in the "Exports Analysis" section here on Bundlephobia).
  3. Instead of the solution in point 2 above, the lodash dependency could be replaced by lodash.pick (if we wanted to keep this instead of implementing point 1 above) and lodash.isequal. This would avoid the ESM caveat mentioned above, and it would also mean that adding new lodash utilities required by this library would be made more of a conscious choice (which would prevent the library size from increasing unexpectedly). It would also mean that the dependencies could be kept as "external" via webpack, which would mean that these dependencies could be shared with other libraries that might use them.

Let me know what you think, and I'll be happy to create a PR for this if you want me to!

To Reproduce

EDIT: https://github.com/joealden/react-xarrows-issue-118-repo

Screenshots

2022-02-04_11-45

Eliav2 commented 2 years ago

When considering whether or not adding lodash as dependency, I actually created production builds (using CRA, which uses webpack) with and without lodash. The difference was about 2-3k in size in production build.

If you using production builds which uses tree shaking(removing unused code) only the section of lodash that is being used will be bundled.

Can you provide more evidence that using react-xarrows will cause adding the entire lodash Lib to be added to the final bundle?

By the way, solution 3 was used in previous versions but it's not efficient as if you look inside the code of each of this "small" packages you will notice they are not small because each utility in lodash relays on another utilities. And adding multiple small lodash packages will cause to the overall bundle size be larger then lodash itself

joealden commented 2 years ago

When considering whether or not adding lodash as dependency, I actually created production builds (using CRA, which uses webpack) with and without lodash. The difference was about 2-3k in size in production build.

If you using production builds which uses tree shaking(removing unused code) only the section of lodash that is being used will be bundled.

Can you provide more evidence that using react-xarrows will cause adding the entire lodash Lib to be added to the final bundle?

@Eliav2 please see https://github.com/joealden/react-xarrows-issue-118-repo for a reproduction of this problem with CRA (please read the README for how to reproduce).

Here are the results that I found:

2022-02-10_12-47


By the way, solution 3 was used in previous versions but it's not efficient as if you look inside the code of each of this "small" packages you will notice they are not small because each utility in lodash relays on another utilities. And adding multiple small lodash packages will cause to the overall bundle size be larger then lodash itself

@Eliav2 yeah that isn't ideal, but it would be better than bundling all of lodash (if we would only be using 1 or 2 of these packages)! Another solution that I just thought of (that should work) would be to keep the current dependency on lodash, but use "scoped imports", meaning making the following changes to the relevant files:

- import _ from "lodash"
+ import pick from "lodash/pick"
+ import isEqual from "lodash/isEqual"

This should mean that common code is still shared, but any dead code can be tree shaken.

Eliav2 commented 2 years ago

thank for this great report @joealden

most of the lib was rewritten(but nothing pushed yet to github) so pull request can't be merged easily, but i will consider bundle size more seriously on next release. thank you