WordPress / gutenberg

The Block Editor project for WordPress and beyond. Plugin is available from the official repository.
https://wordpress.org/gutenberg/
Other
10.52k stars 4.21k forks source link

Remove lodash dependency from WordPress packages. #17025

Closed nerrad closed 1 year ago

nerrad commented 5 years ago

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:

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:

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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"?

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 commented 5 years ago

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 commented 5 years ago

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:

adamsilverstein commented 5 years ago

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 commented 5 years ago

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.

ocean90 commented 5 years ago

Previously: https://github.com/WordPress/gutenberg/pull/9374 and https://github.com/WordPress/gutenberg/pull/9374#issuecomment-479761360

sainthkh commented 4 years ago

I tested babel-plugin-lodash in #21277. But it somehow increased the bundle size. I leave the comment here for future reference.

gziolo commented 3 years ago

There is a related issue opened by @mdmag in #27982 that covers a very similar topic:

I love the @wordpress/data package so much because it really solved major problems at my application and I think it should gain more popularity among the React community. But hey, once I import it, my app size increases drastically. @wordpress/data package alone is 33kb at the moment. Here is the list of its dependencies: 1- lodash 73Kb 2- @wordpress/compose 34kb 3- @wordpress/redux-routine 10kb 4- @wordpress/hooks 6kb 5- @wordpress/deprecated 2kb 6- @wordpress/is-shallow-equal 2kb 7- @wordpress/priority-queue 2kb 8- @wordpress/escape-html 2kb Around 164kb to just use @wordpress/data .. I hope this would be reduced. I think that lodash is a major part here. I hope to replace the the functions used from lodash with native js implementation. Here is the list of lodash functions used in @wordpress/data , @wordpress/compose and @wordpress/redux-routine: { forEach, without, debounce, includes, castArray, throttle, camelCase, upperCase, omit, get, mapValues, isObject, has ,flowRight, isPlainObject, map, isString, merge } One short term solution is to change the syntax for lodash import in those packages from:

import { forEach } from "lodash";

to

import forEach from "lodash/forEach";

so if any one uses the package outside WordPress, he may need to have lodash-webpack-plugin but the long term solution is to implement vanilla JS functions instead of lodash functions in those packages: @wordpress/data , @wordpress/compose and @wordpress/redux-routine

I'm also sharing my response left there:

Have you tried replacing lodash with lodash-es in your webpack config? WordPress exposes lodash as a global to be shared between all JS modules so it doesn't need to optimize for the tree-shaking scenario, but in your case, it might help even it initially this package is a bit larger according to https://bundlephobia.com/result?p=lodash-es@4.17.20.

I also think that @ZebulanStanphill did some efforts in the past to use native functions instead of their lodash alternatives. It would definitely help here as well.

Aside: @wordpress/compose is tree-shakeable by default, and it only pulls createHigherOrderComponent and pure methods in @wordpress/data. It happens mostly when you use withSelect and withDispatch which is less popular option than React hooks these days.

tyxla commented 2 years ago

I'm planning to start working incrementally on removing Lodash completely from the repository. Usage is not so high, and the bundle size impact will be huge for some of the packages when used externally.

andrewserong commented 2 years ago

I'm planning to start working incrementally on removing Lodash completely from the repository.

Nice idea taking this on @tyxla! I was wondering if it's worth having a linting rule or something that prevents the methods you've removed from being re-imported? (I'm just imagining the potential whack-a-mole in case other PRs attempt to reintroduce any of the function imports you've removed 😅) Apologies if this has been discussed already!

tyxla commented 2 years ago

Nice idea taking this on @tyxla! I was wondering if it's worth having a linting rule or something that prevents the methods you've removed from being re-imported? (I'm just imagining the potential whack-a-mole in case other PRs attempt to reintroduce any of the function imports you've removed 😅) Apologies if this has been discussed already!

Good call, thanks! It's been on my mind and I'm glad you brought it up! See https://github.com/WordPress/gutenberg/pull/41651 - the idea is to maintain that and grow it as we remove more and more of those methods.

gziolo commented 2 years ago

Resharing my comments from PRs for visibility.

https://github.com/WordPress/gutenberg/pull/42467#issuecomment-1186863403

We need to ensure that new utilities introduced replacing lodash alternatives don't increase the overall size of JavaScript code in WordPress core. It should at least remain at the size of lodash when the entire refactoring is concluded.

https://github.com/WordPress/gutenberg/pull/42471#issuecomment-1186879992

At the same time, we should consider putting new utilities replacing functionality from lodash in their own packages and making them internal. The fun part is that they would get duplicated in the context of WordPress core, similar to classname, memoize or rememo.

The difficulty here is that both comments contradict at first glance 😅I don't think there is a clear way moving forward, we just need to optimize for the usage of WordPress packages both inside WordPress core and outside.

tyxla commented 2 years ago

Thanks, @gziolo, for bringing those up. Let's keep this issue open until we resolve those problems, as they're part of the responsible way to address the Lodash migration.

spacedmonkey commented 2 years ago

There are number of uses includes, that should be extremely simple to change to es7 includes.

tyxla commented 1 year ago

A year later, after a few hundred PRs, we're finally there! #52561 removes the very last usage within a @wordpress package!

There are some de-duplications and optimizations we can work on, but they're not a requirement for closing this one and I'll be working on them afterwards.