-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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 _.values()
#41905
Conversation
I see there's already someone from the native mobile (RN) side of things on the reviewers list so, I'll go ahead and remove myself. @tyxla , let me know if you have something specific in mind for me, thanks! |
No problem, sounds good, thanks 👍 |
Size Change: -14 B (0%) Total Size: 1.25 MB
ℹ️ View Unchanged
|
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.
Code changes lgtm and tests are passing. I'll defer to others on the React Native side of things though!
Going to ship as the RN part is covered by tests and they're passing. |
I wanted to provide a follow up that I tested this on an iPhone SE (iOS 11) and Samsung Galaxy S20 (Android 12). I did not encounter any new issues, but did capture two regressions unrelated to these changes. |
Thanks a lot @dcalhoun 🙌 🙇 |
What?
Lodash's
values()
is used only a few times in the entire codebase. This PR aims to remove that usage. We also remove a couple of simple usages that are used in the affected filesWhy?
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?
Removing
_.values()
is straightforward in favor of usingObject.values()
. We're also removing a few Lodash usages that are replaced easily with their native counterparts -reduce()
,map()
,includes()
, andsome()
.Testing Instructions
npm run test-unit packages/edit-post/src/store
Questions
Since this touches a react native version of a component, should it have a changelog entry @mirka @ciampo?