-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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 _.set()
in PushChangesToGlobalStylesControl
#52404
Conversation
Size Change: +80 B (0%) Total Size: 1.42 MB
ℹ️ View Unchanged
|
Flaky tests detected in f9d5dc565f027ee40934f4c8764a24fa5d651c70. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5484348128
|
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.
The refactoring doesn't seem to work for me.
Tested with Blank page template and Heading blocks.
Screencast
CleanShot.2023-07-07.at.12.45.02.mp4
setImmutably( newBlockStyles, path, undefined ); | ||
setImmutably( | ||
newUserConfig, | ||
[ 'styles', 'blocks', name, ...path ], | ||
value | ||
); |
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.
Unless I'm missing something, this shouldn't work. Unlike lodash#set
, our helper returns a new object instead of updating the passed argument.
I think return values should be assigned as new config values.
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.
Yup, you're right.
I'm actually looking at this and it seems that the intention was to mutate the original object. Will have to address it in a different way to keep the mutation.
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.
@Mamaduka if you have a chance, take a look at the latest implementation.
We're duplicating setNestedValue
and I'm going to follow-up with deduplicating it afterward.
f9d5dc5
to
88de895
Compare
88de895
to
5e5723b
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.
The refactoring works great now in my manual tests 🎉
What?
This PR removes Lodash's
_.set()
from thePushChangesToGlobalStylesControl
component.With #52278 and #52279, this removes the last occurrence of
_.set()
!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 the usage with the existing
setNestedValue
utility function, which we're temporarily duplicating.Testing Instructions
The typography, spacing, dimensions, and color changes you made should now apply to all blocks of that type. You can verify this by navigating to Global Styles → Blocks → "Your Block".