Lodash: Refactor away from _.words()
#42467
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tyxla
added
[Feature] Inserter
tyxla
requested review from
aristath,
gziolo,
swissspidy,
jorgefilipecosta,
chad1008,
andrewserong,
ntsekouras and
fabiankaegy
|
Size Change: +2.07 kB (0%) Total Size: 1.27 MB
ℹ️ View Unchanged
|
talldan
reviewed
Jul 18, 2022
| @@ -1,7 +1,9 @@ | |||
| /** | |||
| * External dependencies | |||
| */ | |||
| import { deburr, find, words } from 'lodash'; | |||
| import { noCase } from 'change-case'; | |||
There was a problem hiding this comment.
noCase could be overkill given the string being operated on is human search text. Seems like the main thing it does is insert spaces into camel cased text, but I don't expect humans will write that way (even those of us that are coders 😄).
edit - ah, I missed that this is also used on the block name, github had collapsed that part of the code, you can ignore this comment!
There was a problem hiding this comment.
Thanks for the feedback - FWIW _.words() is pretty costly, and while I haven't directly compared the costs with noCase(), I'm almost certain that noCase() will be cheaper. Can thoroughly compare if you have doubts, though.
Did you have any other reservations with the changes suggested in this PR, @talldan?
There was a problem hiding this comment.
You're all good. This was just a passing comment while I was reviewing #42459, which discussed this file.
|
Not a blocker for this PR, but a general note to take into account so we can address it in a systematic way.
This comes at the small cost in
However, in the context of WordPress core it gets multiplied by every package/entry point that uses it: #41865 (
#42429 (
|
This was referenced Jul 18, 2022
|
Thanks for the feedback @gziolo! As we progress with the Lodash migration, including a package multiple times is an inevitable side effect. However, it could mostly be addressed as we identify which those are, and then we could externalize some of them, and introduce them as vendor scripts in the WP core. I think this makes sense as part of #17025 and I can see you already brought those points up there. In any case, I think we should be able to go ahead with this one, and deal with the multiplication problem separately, resting assured that it's under our radar. I'm happy to devote some time to that, and including to some of the other packages that occur often (like |
|
@tyxla, you probably didn't notice that I edited my comment to make it clear that I don't think it's a blocker for this PR. 6kb is still less than 24kb from |
👍 Fully agree on this one - it's part of the journey - as we iterate and progress with the migration, opportunities for optimization will arise, and we should be consciously grasping them. Again, thanks for bringing that up, awareness is the game changer here. |
tyxla
force-pushed
the
remove/lodash-words
branch
from
e63e974 to
140edad
Compare
sgomes
approved these changes
Aug 10, 2022
| * @return {Array} Words, extracted from the input string. | ||
| */ | ||
| function extractWords( input = '' ) { | ||
| return noCase( input ).split( ' ' ).filter( Boolean ); |
There was a problem hiding this comment.
Nit: I would prefer .filter( s => !!s ), .filter( s => Boolean( s ) ), or .filter( s => s !== '' ), as I find them much more readable, but I concede that .filter( Boolean ) usage is pretty widespread, and has probably become an idiom at this point.
There was a problem hiding this comment.
Yeah, we're doing that in quite a bunch of places in Gutenberg already, and it's already become idiomatic in this codebase IMHO.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
This PR removes the
_.words()usage completely and deprecates the function. It also removes one of the last remaining_.deburr()usages in the same file in favor ofremove-accentsthat we've been using in other packages already.Why?
Lodash is known to unnecessarily inflate the bundle size of packages, and in most cases, it can be replaced with native language functionality. See these for more information and rationale:
@wordpress/api-fetchpackage haslodashas a dependency #39495How?
Similar to #42427, we're suggesting to use the
change-caselibrary, which offers modular functions for all the casing functions we use from Lodash (likesnakeCase,capitalize,startCase,camelCase,kebabCase), at a small price (the methods are small themselves), and has TS support. The benefit of using that external library for all those case conversion functions is that we won't have to maintain our own versions of them, and those are already broadly used and tested.In particular, we replace the single instance of
words()with a custom implementation that usesnoCase()to generate a normalized string of words, then splits it into an array and filters out the empty values.We also use the opportunity to remove one of the last remaining
_.deburr()usages in that same file, since it touches the same functionality we're testing here.Testing Instructions
npm run test-unit packages/block-editor/src/components/inserter/test/search-items.jsmédia