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

lodash merge #1928

Closed
obecny opened this issue Feb 12, 2021 · 10 comments
Closed

lodash merge #1928

obecny opened this issue Feb 12, 2021 · 10 comments
Assignees
Labels
Discussion Issue or PR that needs/is extended discussion.

Comments

@obecny
Copy link
Member

obecny commented Feb 12, 2021

Just bundled today some web project ( 3 plugins, collector exporter, zone.js, ) and I see something like this

asset otel.min.js 172 KiB [emitted] [minimized] (name: otel)
runtime modules 1.04 KiB 5 modules
modules by path ./node_modules/@opentelemetry/ 420 KiB 192 modules
./www/index.js 1.22 KiB [built] [code generated]
./node_modules/zone.js/fesm2015/zone.js 124 KiB [built] [code generated]
./node_modules/lodash.merge/index.js 50 KiB [built] [code generated]
./node_modules/shimmer/index.js 2.87 KiB [built] [code generated]
webpack 5.21.2 compiled successfully in 3098 ms

Seems a lodash merge is quite big, I thought it should be much smaller, especially that we only use it for merging config, but it looks like there is just one file and it is quite big.

@obecny obecny added bug Something isn't working and removed bug Something isn't working labels Feb 12, 2021
@dyladan
Copy link
Member

dyladan commented Feb 17, 2021

lodash.merge bundles the single function but also its dependencies like isObject, baseMerge, and others from lodash. It seems like it handles a lot of corner cases, but maybe we don't need such a powerful merge.

Perhaps use merge-deep instead? Looks like it is a significantly smaller package (1 js file 1.32 kB before minification). It likely doesn't handle all the same corner cases, but it is probably good enough for our basic config merge use.

edit: actually merge-deep has its own dependencies so it might not be the way to go.

@obecny
Copy link
Member Author

obecny commented Feb 18, 2021

merge-deep is quite old but it looks very solid and is used widely, we should probably give it a try.

@Flarna Flarna added the Discussion Issue or PR that needs/is extended discussion. label Feb 18, 2021
@legendecas
Copy link
Member

https://www.npmjs.com/package/extend is another alternative in the case of merging configs, and it has no dependencies ( merge-deep depending on 3 other packages makes it larger once bundled).

@dyladan
Copy link
Member

dyladan commented Feb 19, 2021

npmjs.com/package/extend is another alternative in the case of merging configs, and it has no dependencies ( merge-deep depending on 3 other packages makes it larger once bundled).

Looks like this (~23k) is smaller than lodash.merge (~50k). Has anyone checked a bundle with deep-merge to see what the final bundle size difference is?

@obecny
Copy link
Member Author

obecny commented Feb 19, 2021

merge-deep is the smallest, you have size on npm
https://www.npmjs.com/package/merge-deep
8.46 kB with it's dependency all together, so they are very small too

@dyladan
Copy link
Member

dyladan commented Feb 19, 2021

8.46 doesn't include dependency its just the size of unzipping the tar.gz

These sizes are pure install sizes with dependencies no tree shaking or bundling.

merge deep 16 packages 288k
lodash.merge 1 package 64k
extend 1 package 44k
➜  test npm i merge-deep
+ [email protected]
added 16 packages from 16 contributors and audited 16 packages in 2.535s
found 0 vulnerabilities
➜  test du -hs node_modules 
288K	node_modules

➜  test rm -rf node_modules 
➜  test npm i lodash.merge
+ [email protected]
added 1 package from 2 contributors and audited 1 package in 0.445s
found 0 vulnerabilities
➜  test du -hs node_modules
 64K	node_modules

➜  test rm -rf node_modules 
➜  test npm i extend
+ [email protected]
added 1 package from 2 contributors and audited 1 package in 0.569s
found 0 vulnerabilities
➜  test du -hs node_modules
 44K	node_modules

@obecny
Copy link
Member Author

obecny commented Feb 20, 2021

If you check merge-deep you will see that it is one file
https://github.com/jonschlinkert/merge-deep/blob/master/index.js 1,32KB
that has only 5 dependencies in total (including dependency of dependency). They are all one file repo and very small
These are

  1. arr-union - https://github.com/jonschlinkert/arr-union/blob/master/index.js - 527B
  2. clone-deep - https://github.com/jonschlinkert/clone-deep/blob/master/index.js 1023B
  3. is-plain-object - https://github.com/jonschlinkert/is-plain-object/blob/master/is-plain-object.js 739B
  4. kind-of - https://github.com/jonschlinkert/kind-of/blob/master/index.js 3,48KB
  5. shallow-clone- https://github.com/jonschlinkert/shallow-clone/blob/master/index.js 1,84KB

Counting together 9088 Bytes

not sure how did you come out with 288kB.

@legendecas
Copy link
Member

legendecas commented Feb 20, 2021

Counting together 9088 Bytes

From the Github file size stat of https://github.com/justmoon/node-extend/blob/main/index.js the single file package 'extend' (no dependencies) has its size as 3.28KB, which sounds kinda smaller than 'merge-deep' but not significant anyway.

@dyladan
Copy link
Member

dyladan commented Feb 22, 2021

Personally, I prefer to use something which doesn't have its own dependencies to keep our dependency tree as small as possible.

@legendecas
Copy link
Member

Closing as the dependency on lodash.merge has been removed from the code base.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Discussion Issue or PR that needs/is extended discussion.
Projects
None yet
Development

No branches or pull requests

4 participants