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

refactor(v2): use single method package instead of whole Lodash package in blog plugin #2460

Merged
merged 1 commit into from
Mar 25, 2020

Conversation

Simek
Copy link
Contributor

@Simek Simek commented Mar 24, 2020

Motivation

Currently Lodash in this repository is quite freely used without much care of it's footprint on the bundles/packages sizes. This is the first PR in a series which aims to improve this aspect.

This PR replace @docusaurus/plugin-content-blog Lodash import with single method package lodash.kebabcase because only this method was used from the whole package across all files.

I would like to improve other packages but before I can do that I need an answer in which way you would like me to handle packages that are using multiple Lodash methods. There are two ways:

  • instead of lodash include few, single method packages (like in example above)
  • maintain lodash in the package but use cherry picking for the packages

    e.g.: import flatten from 'lodash/flatten';

Personally I would also consider opting-out Lodash completely in favor of native code.

Have you read the Contributing Guidelines on pull requests?

Yup.

Test Plan

Same test result before and after package swaps.

Related PRs

None at this time.

@Simek Simek requested a review from yangshun as a code owner March 24, 2020 11:19
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Mar 24, 2020
@docusaurus-bot
Copy link
Contributor

Deploy preview for docusaurus-2 ready!

Built with commit 658341a

https://deploy-preview-2460--docusaurus-2.netlify.com

Copy link
Contributor

@lex111 lex111 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I definitely like this, although it does not affect the size of bundles (for end-users), but of course I would not completely refuse lodash, why?

@lex111 lex111 changed the title feat(v2): use single method package instead of whole Lodash package in blog plugin refactor(v2): use single method package instead of whole Lodash package in blog plugin Mar 24, 2020
@lex111 lex111 added the pr: polish This PR adds a very minor behavior improvement that users will enjoy. label Mar 24, 2020
@lex111 lex111 added this to the v2.0.0-alpha.49 milestone Mar 24, 2020
@Simek
Copy link
Contributor Author

Simek commented Mar 24, 2020

I would argue that this affects the end users bundle because you are not using cherry picking while importing methods nor any Babel plugin to do it you you automatically. Second reason why it affects end user is that whole Lodash library is listed in dependencies which are downloaded every time user installs package. It also affects installation time.

In opting-out topic: Lodash has been created several years ago when there was no ES6, ES7, ES10 etc. A lot of feature has been added to JS through those years allowing users to archive same results without any third-party package. Also these days native code could be in some cases X times faster because of Lodash wrapping, checks and some unnecessary operations according to current standards. In my opinion Lodash overall don't fit well high modularity and chunk packages era.

For more information please visit: https://github.com/you-dont-need/You-Dont-Need-Lodash-Underscore

And please address my question about packages that are using multiple Lodash methods.

@yangshun
Copy link
Contributor

yangshun commented Mar 24, 2020 via email

Copy link
Contributor

@yangshun yangshun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@yangshun
Copy link
Contributor

Please note that we are not shipping Lodash into the final client bundle. Lodash is only used in the local development step and is not part of the final build artifact output. I would use native methods if they're available but in most of the cases we're using it I believe there's no native equivalent.

@yangshun yangshun merged commit 85c124e into facebook:master Mar 25, 2020
@Simek
Copy link
Contributor Author

Simek commented Mar 25, 2020

@yangshun What about packages with multiple Lodash methods? Can you answer my question from the first post in this PR? As I wrote in the first message I can work on that also, but I need you to choose the way.

Also yesterday I was working on Windows where a lot of NPM command fails so I cannot provide packages bundles size comparison. Today I going to setup dev on my Mac and I will try to fully verify effect of this change.

@yangshun
Copy link
Contributor

Both approaches are fine. I think the way you did in this PR is good.

Just to be clear, the package size you mean here is npm package size which only affects developers, not the size of Docusaurus websites right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA pr: polish This PR adds a very minor behavior improvement that users will enjoy.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants