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

Why are lodash imports as named imports? This adds the entire lodash to the dist #9778

Closed
lgraziani2712 opened this issue Feb 6, 2020 · 9 comments · Fixed by #9787
Closed

Comments

@lgraziani2712
Copy link
Contributor

Describe the bug
Lodash is being imported as if was an es-module, but it isn't, so is adding me the entire lodash/lodash.js file plus all the specific modules I'm importing.

To Reproduce
Steps to reproduce the behavior:

  1. Generate stats when building.
  2. Open webpack stats with https://webpack.github.io/analyse
  3. Go to the modules section and search "lodash/lodash.js". Click in the green button to analyze that specific module.
  4. See how many storybook modules are importing lodash as a es-module.

Expected behavior
To not have the entire lodash module plus each lodash function in the final bundle.

Screenshots
image

Code snippets
This could be solved by importing each lodash modules specifically. E.g:

Instead of:

import { isNil } from 'lodash';

Would be:

import isNil from 'lodash/isNil';

Additional context
I would gladly fix it in a PR if its OK.

Thank you!

@shilman
Copy link
Member

shilman commented Feb 7, 2020

cc @patricklafrance

@shilman
Copy link
Member

shilman commented Feb 7, 2020

@lgraziani2712I'm fine with this change and would appreciate a PR fixing this across the board. 🙏

@patricklafrance you told me that it should be tree-shaken out of the build, but it sounds like that's not working and it's easier to change it in our code than to force our users to get tree-shaking working in theirs. please speak up if you think this is a bad idea!

@patricklafrance
Copy link
Member

patricklafrance commented Feb 7, 2020

I might be wrong, this is hard to test since most of storybook dependencies also have a dependency on lodash and it might be these dependencies who are importing a complete version of lodash.

I dont mind changing this.

@patricklafrance
Copy link
Member

patricklafrance commented Feb 7, 2020

Haven't test if it's right but I keep reading the following:

Tree-shaking is syntax-sensitive. To enable tree-shaking in lodash, you have to import functions using syntax like import foo from 'lodash/foo'. import { foo } from 'lodash' will not tree-shake, nor will, obviously, import _ from 'lodash'. Support for this syntax was implemented in Lodash v4.
If you are using Webpack v4, you don’t need to use lodash-es. The lodash-es library supports tree-shaking out of the box because it uses ES modules. However, with lodash v4, tree-shaking works without additional configuration in Webpack v4. It is also worth noting is that if you use lodash-es and you have other dependencies that require lodash, both will end up in the app bundle.

@lgraziani2712
Copy link
Contributor Author

Hmm I think what that citation says is that importing things like import bla from 'lodash/bla' works as if you used const bla = require('lodash/bla') but not if you use import { bla } from 'lodash'.

@shilman
Copy link
Member

shilman commented Feb 7, 2020

@lgraziani2712 thanks for putting together the fix. i'm going to merge and release your PR after CI finishes. Hoping you can verify the fix after it's released! 🙏

@shilman
Copy link
Member

shilman commented Feb 7, 2020

@lgraziani2712 released on 6.0.0-alpha.8

@lgraziani2712
Copy link
Contributor Author

lgraziani2712 commented Feb 7, 2020

I can confirm the fix! My bundle now has half the size for the lodash module:

Before:

image

After:

image

@shilman
Copy link
Member

shilman commented Feb 8, 2020

Awesome @lgraziani2712 !!! 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants