Lodash: Refactor away from _.setWith()
#47017
Conversation
tyxla
added
[Type] Performance
tyxla
requested review from
Mamaduka,
sgomes,
aristath,
jsnajdr,
swissspidy,
flootr,
jorgefilipecosta,
chad1008,
ntsekouras,
fabiankaegy and
brookewp
|
Size Change: +4.6 kB (0%) Total Size: 1.33 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 2635c75. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/3959670242
|
| * Clones all nested objects in the specified object. | ||
| * | ||
| * @param {Object} object Object to set a value in. | ||
| * @param {number|string|Array} path Path in the object to modify. |
There was a problem hiding this comment.
I would prefer to see the string-based API removed altogether; parsing strings to split them into paths is quite inefficient if it ends up in a hot path. I've had to fix several instances of this in Redux selectors over the years (in other applications), and the fix was usually to move to a (static) array path instead.
There's also the matter of this API being more complex and less explicit, which makes it difficult to migrate to something simpler in the future, even if the number and string notations aren't being used. If at this time it's easy to identify all the usage and move to something simpler, then we should take the chance, lest it become more difficult to do so in the future.
The only argument in favour of the dot notation, as I see it, is readability; but I don't think it's worth the other issues, given that an array of path segments is pretty readable on its own. Plus, if we really wanted to keep dotted paths for some reason, we could always relegate that to the consumer, where it makes sense:
import { immutableSet } from 'hooks/utils';
const THE_PATH = 'foo.bar.baz'.split( '.' );
export function doSomething( input ) {
return immutableSet( input, THE_PATH, 42 );
}The above avoids parsing the path and instancing an array on every call, and is just as readable as passing it directly to immutableSet.
All of that said, I acknowledge this is possibly out of scope for this PR.
There was a problem hiding this comment.
Thank you for the observation and your hint, @sgomes! You actually caught one of my biggest doubts about this work here.
I was very close to removing the . support, and likely should have, since we have no present usage of it in the codebase. That will simplify the code and add less complexity to an already complex part of the codebase.
I consider this part of the PR's scope, since we're directly tinkering with the API, so it's the right time to simplify it.
I'll work on that before asking you to do another review.
There was a problem hiding this comment.
Removed support for dot notation in 2635c75.
| @@ -30,8 +30,75 @@ export const cleanEmptyObject = ( object ) => { | |||
| return isEmpty( cleanedNestedObjects ) ? undefined : cleanedNestedObjects; | |||
| }; | |||
|
|
|||
| /** | |||
There was a problem hiding this comment.
It looks like we're dropping support for arrays and their indexed notation [0] here. Is that intentional?
There was a problem hiding this comment.
I'm happy to implement it, but IMO it would needlessly complicate the function, and I didn't see a need for that within our existing usages.
Unless I'm misunderstanding you, in which case I'd ask you for a unit test or an example of what exactly you mean 😉
There was a problem hiding this comment.
Nope, dropping it is indeed my preferred approach, as is dropping the dot notation as well 👍 Just wanted to make sure it was intentional 🙂
There was a problem hiding this comment.
Thanks for double-checking 👍
|
Hey, @sgomes I've dropped the dot notation support and this is ready for another look when you get a chance. Thanks! |
| * 2 => [ '2' ] | ||
| * [ 'foo', 'bar' ] => [ 'foo', 'bar' ] | ||
| * | ||
| * @param {number|string|Array} path Path | ||
| * @param {string|number|Array} path Path |
There was a problem hiding this comment.
I'd go a step further and enforce it as an array, always, but this is probably a good middle-ground between convenience and simplicity.
There was a problem hiding this comment.
Yeah, I think that could be a good improvement for the future, but I decided to leave that support since we have at least one existing use case for a single string path:
|
Thank you, @sgomes 🙌 |
My initial research resulted that every library we could use for that would be larger than what we're introducing here. Immer also seems to be 16.3KB minified, which isn't too much, but still is more than we're introducing here. I'm happy to try it out if you think it will be better. Given that code-size-wise |
This is in part predicated on the idea that we'd never want to get rid of If the problem we're solving can be dealt with in 60 lines of code, I think that's definitely worth keeping around, particularly since @tyxla was diligent enough to add extensive testing. |
|
It's not the first function that I see around that replicates a lodash function. I remember seeing or having to implement omit in a couple places too. I think this should be seen as a devx issue that needs solving. We can't keep reinventing the same functions forever. I'm fine if we provide them but this function is randomly placed in a random package in a random folder and it has nothing to do with that package. Unless you know it's there, you're not going to use it. In fact I wrote a lot of code in the I don't care whether it's a third-party package or a first-party one but seeing all these utility functions scattered everywhere is a problem. |
|
I generally agree with your sentiment, @youknowriad. At the same time, my opinion is that while you're right, sometimes those functions are domain- or context-specific and way simpler than they would have been if they cover all generic cases. We never wanted to reinvent the wheel, and having a smaller inline abstraction actually makes it more comprehensible what the expectations of input and output of a certain function or component are. Lodash functions would often conceal those abstractions and that made the code less readable, worse typed, and potentially more error-prone for future iterations. Also, I believe that we have far too few of those helper functions inline in packages. Especially considering how much Lodash usage we've removed. I definitely would like to remove them all, but sometimes there's no good candidate for external package, or the use case is way too simple to justify using another external package with additional own dependencies. I do believe the code is in a much better, predictable, and manageable state without Lodash, compared to how it was with Lodash. This doesn't mean that there aren't further opportunities for improvement. I'm always happy to work on those, so thank you for sharing your thoughts. I often like reminding ourselves that this is a marathon and not a sprint. We are making incremental steps toward a better codebase. As we continue to iterate, we'll keep seeking opportunities to improve any devex gaps like the one you mentioned. |
What?
This PR removes Lodash's
_.setWith()from the block editor hooks. There is just a single usage in that block and conversion is pretty straightforward.We're also removing one of the few remaining
_.clone()usages together with that.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 extending the custom immutable set functionality to use custom inline cloning and setting functions. We're adding tests to ensure that we're covering all the necessary prior functionality.
Alternatively, we could introduce new libraries, but since we were handling our own custom function for immutable settings, I thought it makes sense to just expand it.
We could remove the dot notation since it doesn't seem to be necessary for the existing use cases. More or less, it would remove a couple of lines of the path normalization and a few tests. I decided to keep it, but let me know if you'd suggest otherwise.
Another thing we could simplify is to default to an empty object in other places and keep the
immutableSetsimpler in terms of how it handles nullish values, but I decided to keep the previous behavior there.Testing Instructions