-
Notifications
You must be signed in to change notification settings - Fork 4.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Lodash: Refactor away from _.isObject()
#42336
Conversation
Size Change: +10 B (0%) Total Size: 1.25 MB
ℹ️ View Unchanged
|
87244df
to
1cd8a69
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just noting two small differences with lodash
's isObject
:
- in
lodash
, thenull
check isn't performed with strict equality - in
lodash
,isObject
returnstrue
also for functions
It doesn't look like neither of those differences will affect this PR, though.
function isObject( object ) { | ||
return object !== null && typeof object === 'object'; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On which basis did we decide when to refactor the code "inline" vs creating a isObject
utility function?
Given the subtlety of this check (which includes the null
edge case), I'm thinking if it would be better to always refactor the logic to a separate isObject
function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On which basis do we decided when to refactor the code "inline" vs creating a isObject utility function?
Depending on the number of usages in a file, essentially. More than a couple of usages makes it easier and less repetitive to have a separate function.
Given the subtlety of this check (which includes the null edge case), I'm thinking if it would be better to always refactor the logic to a separate isObject function?
I'd disagree, while it adds verbosity, I don't see why we'd always want to keep it a separate function. I think it's enough that we are specifically explicitly excluding the null
case, and it's a JavaScript quirk, so not much we can do about it.
Thanks for the feedback, @ciampo 😉 makes a lot of sense to go through those.
That's fair, however:
Yes, thanks for bringing that up. I've actually manually audited these instances and saw that we never seek to invoke the results anywhere. I think this demonstrates one of the big problems with Lodash - concealing behavior, and sometimes even illogical behavior. Who could guess that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the explanation, @tyxla !
LGTM 🚀
1cd8a69
to
c827208
Compare
What?
This PR removes the
_.isObject()
usage completely and deprecates the function.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-fetch
package haslodash
as a dependency #39495How?
We're replacing it with the safe replacement
typeof === 'object'
ortypeof !== 'object'
, plus checking for thenull
case, becausetypeof null
isobject
as well.Testing Instructions