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

Remove lodash dependency from WordPress packages. #17025

Open
nerrad opened this issue Aug 13, 2019 · 8 comments

Comments

@nerrad
Copy link
Contributor

commented Aug 13, 2019

Right now lodash is used throughout various @wordpress packages in this repo because it's convenient and handles a lot of mundane things/logic that aren't worth duplicating. However there is a cost to using it:

  • Size: lodash.min.js is 73.3kb
  • Increased potential for conflicts with plugins/themes also using lodash.

For this issue, I'd like to propose that we eliminate lodash usage across all our repository packages to reduce/remove the cost incurred by it. To do so:

  • Implement some sort of utility package used for wrapping and exporting specific lodash (while not depending on it as an external). Things like _.kebabCase or _.isPlainObject might be good candidates for this package. Difficulty here would be how to name this package and clearly defining what goes in it (we don't want it becoming a "catch all" type of package.
  • Replace usage of lodash functions that can be implemented via native js and utilize the new "utils" package for others that have logic we don't want to duplicate in all @wordpress/* packages.

Initially, I've started with @wordpress/element dependency here.

I'm willing to spearhead this and get it completed, but before I begin, here's some decisions that we should make to move forward on this issue:

  1. Is removing lodash dependency something we want to do? Everywhere or just select packges?

  2. Should we create a new package to contain specific lodash functions (like _.kebabCase) that can be used where needed instead of lodash? If yes, what name should we give the package. What conventions should we describe for what is allowed in the package?

@swissspidy

This comment has been minimized.

Copy link
Member

commented Aug 13, 2019

It would be good to have some usage analysis across the different packages. My assumption is that for some packages it would be easier to remove lodash than for others. Then, we should have a better understanding of which specific functions we need to keep (whether that's in a separate package or not)

@nerrad

This comment has been minimized.

Copy link
Contributor Author

commented Aug 13, 2019

It would be good to have some usage analysis across the different packages.

Good point and I think that will be useful in helping inform any decision. I'll take on tackling that first.

@talldan

This comment has been minimized.

Copy link
Contributor

commented Aug 14, 2019

Is the idea to import any lodash functions that will continue to be used (i.e. kebabCase) as an individual import? I know there are a lot of ways of doing that. This might be an option:
https://www.npmjs.com/package/babel-plugin-lodash

There's also lodash-es, which should enable proper tree-shaking:
https://www.npmjs.com/package/lodash-es

@youknowriad

This comment has been minimized.

Copy link
Contributor

commented Aug 14, 2019

I personally don't see a lot of value in removing lodash. Removing lodash basically means replacing its functions with custom built ones? What's the difference between a custom utility package and lodash aside "not invented here"?

  • In WordPress context, lodash is not duplicated, it's only loaded once so it can't be considered a problem for bundle size. At least I'm not sure building our own will change anything here.
  • For npm consumers, there are a lot of techniques to extract lodash, bundle it once (this is the default since we use the same version)...

I agree with @talldan that if we were to change something, we should consider loading and bundling specific lodash functions. But that said, in the WordPress context, it's not a perfect solution because we'll end up with duplicated functions across bundles which might not be better in terms of bundle size.

I know I'm sounding "negative" here but do we really have a problem with lodash right now?

@swissspidy

This comment has been minimized.

Copy link
Member

commented Aug 14, 2019

Removing lodash basically means replacing its functions with custom built ones?

Not necessarily. For many utilities there are native replacements. IMHO, if within a package all lodash-helpers can easily be replaced with native solutions (e.g. each -> Array.prototype.each), without having to write a custom one, the it makes sense to drop an unnecessary dependency. Even if that means that there is no change in wp-admin, as lodash will be loaded anyway.

@youknowriad

This comment has been minimized.

Copy link
Contributor

commented Aug 14, 2019

Yes, that makes sense @swissspidy and I'm all for native solutions and I'm all for explorations to reduce our reliance on lodash but there are a lot of cases where it may seem that lodash is useless but in fact, it is:

  • To a degree, a lot of lodash functions are also very small and easy to recreate. We could be tempted to say, let's avoid lodash for these. => we'll recreate the same function over and over
  • .each is a good example: while often we can just replace with native solutions. Lodash versions of these functions handles nullable|undefined values properly while the native couterparts might trigger errors forcing you to repeat the same checks over and over.
@adamsilverstein

This comment has been minimized.

Copy link
Contributor

commented Aug 15, 2019

do we really have a problem with lodash right now?

I agree, it is probably not a problem to load all of lodash (once) in wp-admin (and also, remember that we load underscore as well :) which I feel is a problem itself).

The main problems I do see with is forcing developers (and all website visitors) to load all of lodash on the front end when websites use packages like wp.element. Removing the dependence seems especially valuable for packages that are likely to be used on website frontends.

@nerrad

This comment has been minimized.

Copy link
Contributor Author

commented Aug 16, 2019

From the comments here, I agree that the original proposal of removing all dependency on lodash is not necessary. However as @adamsilverstein articulated (and in sync with the original prompt for this discussion), I think it's worthwhile to explore what packages having a dependency on lodash and potentially used on the frontend could have that dependency removed.

However, this still leaves the question for what to do with the few functions that aren't trivial to replace (as noted for wp.element). @talldan mentioned here about using lodash-es, but I'm not sure how that would work given the package build process in the mono-repo (happy to be educated).

I still plan on doing a review of package use of lodash - but in light of the convo here I'll focus on the packages potentially used on the frontend.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.