Lodash: Refactor away from _.keyBy()
#42044
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
[Type] Performance
tyxla
requested review from
aristath,
gziolo,
swissspidy,
chad1008,
ntsekouras and
fabiankaegy
tyxla
requested review from
ellatrix,
fluiddot,
draganescu,
talldan,
noisysocks,
tellthemachines,
adamziel,
kevin940726 and
nerrad
as code owners
|
Size Change: -6 B (0%) Total Size: 1.25 MB
ℹ️ View Unchanged
|
jorgefilipecosta
approved these changes
Jun 29, 2022
|
Yeah I feel that this one is a big loss to code readability. |
|
I wonder if a for loop would be easier to understand here? (Also slightly faster if that matters.) |
|
Thanks for the comments y'all 😉 It's always valuable to hear some feedback. I can definitely see how It's also fair to say this is one of the simples possible On whether to use a Another reason why I prefer the custom and non-generalized approach here is because there are little details that might differ between usages. For example, what happens if the key we're keying by occurs more than once? Will the first one be kept, or will it be overridden by the second one? Or perhaps we want to keep both, ensuring a unique key? That would depend on the use case. In any case, Lodash's |
adamziel
reviewed
Jun 30, 2022
Comment on lines
+344
to
+353
| const templatePartsById = templateParts | ||
| ? // Key template parts by their ID. | ||
| templateParts.reduce( | ||
| ( newTemplateParts, part ) => ( { | ||
| ...newTemplateParts, | ||
| [ part.id ]: part, | ||
| } ), | ||
| {} | ||
| ) | ||
| : {}; |
There was a problem hiding this comment.
How about a loop instead?
Suggested change
| const templatePartsById = templateParts | |
| ? // Key template parts by their ID. | |
| templateParts.reduce( | |
| ( newTemplateParts, part ) => ( { | |
| ...newTemplateParts, | |
| [ part.id ]: part, | |
| } ), | |
| {} | |
| ) | |
| : {}; | |
| const templatePartsById = {}; | |
| if ( templateParts ) { | |
| for( const part of templateParts ) { | |
| templatePartsById[ part.id ] = part; | |
| } | |
| } |
It's shorter and easier to scan.
There was a problem hiding this comment.
Same for other updated files.
There was a problem hiding this comment.
As explained in my above comment, I prefer the reduce() version because it fits better within the FP-style that's most common in the repo. I honestly see this as a very simple reduce() implementation, and a pretty obvious one, too. Also, I'm not particularly happy with mutating objects - it could introduce subtle bugs in the future, I've seen that before. And just in case that helps, I've added a comment to make the intention clear. So unless you have really strong feelings about this, I'd prefer keeping the reduce() version.
There was a problem hiding this comment.
I have semi-strong feeling about reduce vs loops:
- The loop is shorter
- I can immediately tell what it does without any explanatory comment.
- The reduce implementation required that explanatory comment which tells me it's not clear from just looking.
Personally, I always get confused by all but the simplest reduce() implementations and end up reading the code for a few minutes. There's just something unnatural about the reduce mental model for me specifically – I love FP but at the same time I tend to think imperatively :-)
Let's agree to disagree, I guess. I don't want to block your work so let's get a second opinion here. Maybe @jsnajdr or @gziolo or @mcsf would like to chip in?
There was a problem hiding this comment.
IMO this is definitely a reduce use case and I agree with @tyxla that is a simple one too. The only change I'd recommend that might made it more readable could be to change the previous value to accumulator as we do in many places in our code base and in some places like change the name of the current value to be even more descriptive. For example this: newTemplateParts, part -> accumulator, templatePart
There was a problem hiding this comment.
Also wanted to note that keyBy can also be confusing to some and this includes me. I had to go to lodash docs to see what exactly is for.
There was a problem hiding this comment.
Seems a bit excessive to ping for this. :) As long as care is put into the flow and the naming of things, the question of looping vs. reducing doesn't matter that much. (But I admit that I haven't always felt this way.)
Just to troll a bit, well, why not an even higher-level construct?
const templatePartsById = Object.fromEntries( ( templateParts || [] ).map( p => [ p.id, p ] ) );
noisysocks
approved these changes
Jul 1, 2022
chad1008
approved these changes
Jul 4, 2022
|
Thanks, everyone, for the discussion. I think such conversations are very important to keep the code in a good spot. I also want to say that nothing is final, and everything is subject to further improvement. The results of the migration away from Lodash, I don't expect to be different. We're going to leave things better than they are, but I doubt there will be no way to further improve them in the future. |
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?
Lodash's
keyBy()is used only a few times in the entire codebase. This PR aims to remove that usage.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?
Removing
_.keyBy()is straightforward in favor of anArray.prototype.reduce()that goes through the array elements and builds an object, with the specified key as a key, and the entire object as a value. We're intentionally not touching the co-existing Lodash usage to keep the PR shorter and self-contained. All usages are within stores, and that area is generally well covered by tests.Testing Instructions