Lodash: Remove a bunch of usages from tests #41751
Merged
Conversation
This file contains hidden or 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
chad1008,
fabiankaegy,
gziolo,
ntsekouras and
swissspidy
|
Size Change: +1.33 kB (0%) Total Size: 1.24 MB
ℹ️ View Unchanged
|
Member
Author
|
Would be really cool to merge this sooner, as it touches a bunch of files. It has the least impact because it touches only test files, but it's a nice little win. |
talldan
reviewed
Jun 20, 2022
| * External dependencies | ||
| */ | ||
| import { uniqueId } from 'lodash'; | ||
| export const uniqueId = () => |
Contributor
There was a problem hiding this comment.
I don't know if the replacement function is needed - the usage of uniqueId in these tests could just be replaced with hard-coded id values.
lodash uniqueId is just a counter that counts up from 1 rather than a random number:
So I'd expect every test run to produced the same ids, the first call to uniqueId() would return '1', the second '2' etc.
Member
Author
There was a problem hiding this comment.
Good point. Happy to simplify 👍 Done in 8c4d486
talldan
reviewed
Jun 20, 2022
| instanceId={ uniqueId() } | ||
| label={ 'Padding' } | ||
| /> | ||
| <DimensionControl instanceId={ uuid() } label={ 'Padding' } /> |
Contributor
There was a problem hiding this comment.
I also think using a generated id is probably overkill here too. If there's only one dimension control instance in each tree, the instanceId can just be the same value in each test.
Member
Author
There was a problem hiding this comment.
Good call, updated in 8c4d486 to use the same ID.
talldan
reviewed
Jun 20, 2022
Comment on lines
20
to
28
| const keyBy = ( arr, key ) => | ||
| ( arr || [] ).reduce( | ||
| ( r, x ) => ( { ...r, [ key ? x[ key ] : x ]: x } ), | ||
| {} | ||
| ); | ||
| const omit = ( obj, keys ) => | ||
| Object.fromEntries( | ||
| Object.entries( obj ).filter( ( [ key ] ) => ! keys.includes( key ) ) | ||
| ); |
Contributor
There was a problem hiding this comment.
This is the kind of situation where I personally wouldn't be in favor of replacing lodash usage, because these functions are mostly simple to understand in terms of input and output, but the implementation is quite complex and they're only unit tested by proxy.
I do understand the overall goal here. I just wonder whether there's a better way.
The first one is also not meeting the coding standards, where shortened names like r and x shouldn't be used in favour of a more descriptive alternative. That's at least easily fixable.
edit: this isn't intended to act as a blocker though, just sharing some feedback.
Member
Author
There was a problem hiding this comment.
Thanks for all the candid feedback!
While this is in tests and has little impact, I think what you brought up is a valid concern to bring up and I'm happy to do another pass and simplify it. There's actually some opportunity for that here and I'm happy to grasp it.
Member
Author
There was a problem hiding this comment.
Just pushed a couple of new commits, and I'd love to hear what you think:
- I completely got rid of
omit()in favor of a simpler declaration of the fixtures. It's unnecessary to declare a complex object and then pull it in pieces when we can just declare simpler objects in the first place. - I refactored
keyBya bit to make it simpler in the current context, and more readable.
Let me know what you think, and thanks again!
Member
Author
There was a problem hiding this comment.
Oh, and I hope this new version helps you reconsider this statement:
This is the kind of situation where I personally wouldn't be in favor of replacing lodash usage
😉
Contributor
There was a problem hiding this comment.
Definitely better, thanks for addressing the feedback.
I still feel like some of these functions (e.g. isPlainObject) should be library code rather than having the implementation duplicated across the codebase.
Have there been any alternative explorations to this approach?
Member
Author
There was a problem hiding this comment.
I still feel like some of these functions (e.g. isPlainObject) should be library code rather than having the implementation duplicated across the codebase.
We can, and definitely will abstract some of those into library code as the need comes. isPlainObject in particular might end up being one of those.
I do agree with you, and I am definitely not planning to clutter the codebase with redundant snippet repetitions. If you take #41806 for example, I'm doing the opposite there - using the opportunity to abstract a bit of repeated code and make it better and well tested while removing Lodash from it at the same time.
However, I think it should be fine to take these into small steps, do the improvements incrementally, and not try to overengineer it from the beginning. It's a marathon, not a sprint, after all.
What do you think?
Contributor
There was a problem hiding this comment.
Sounds good, thanks for replying to my concerns. Will give this one the thumbs up.
talldan
approved these changes
Jun 21, 2022
|
|
||
| for ( const variantField of fields ) { | ||
| for ( const constantField of fields.filter( | ||
| ( f ) => ! f === variantField |
Member
There was a problem hiding this comment.
I think there is a typo on this condition, right now it is always false. I submitted a PR at #42280.
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?
There's plenty of usage of Lodash in our tests. This PR aims to remove it in one sweep, because of the little impact all these usages have.
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?
We're updating a bunch of methods, but the usages are mostly straightforward, and we only need the tests to be passing to ensure that the migration went well.
Testing Instructions