-
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: Remove completely from @wordpress/edit-widgets
package
#43682
Conversation
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.
LGTM, thank you @tyxla! 👍
@@ -14,7 +9,7 @@ const { Fill: ToolsMoreMenuGroup, Slot } = createSlotFill( | |||
|
|||
ToolsMoreMenuGroup.Slot = ( { fillProps } ) => ( | |||
<Slot fillProps={ fillProps }> | |||
{ ( fills ) => ! isEmpty( fills ) && fills } | |||
{ ( fills ) => fills.length > 0 && fills } |
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.
Can fills
be undefined?
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 double-checking, but nope, it's always an array:
gutenberg/packages/components/src/slot-fill/slot.js
Lines 70 to 92 in b33b256
const fills = ( getFills( name, this ) ?? [] ) | |
.map( ( fill ) => { | |
const fillChildren = isFunction( fill.children ) | |
? fill.children( fillProps ) | |
: fill.children; | |
return Children.map( fillChildren, ( child, childIndex ) => { | |
if ( ! child || typeof child === 'string' ) { | |
return child; | |
} | |
const childKey = child.key || childIndex; | |
return cloneElement( child, { key: childKey } ); | |
} ); | |
} ) | |
.filter( | |
// In some cases fills are rendered only when some conditions apply. | |
// This ensures that we only use non-empty fills when rendering, i.e., | |
// it allows us to render wrappers only when the fills are actually present. | |
( element ) => ! isEmptyElement( element ) | |
); | |
return <>{ isFunction( children ) ? children( fills ) : fills }</>; |
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.
Well, then I don't understand why we needed the second fills
here 😅? It probably would be safe also to delete it.
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.
Well, then I don't understand why we needed the second
fills
here 😅? It probably would be safe also to delete it.
I left it as-is, but I can open a follow-up PR if you feel strongly about it 😅
Size Change: +1 B (0%) Total Size: 1.24 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.
LGTM 👍 Thanks!
What?
This PR removes all of the Lodash occurrences from the
@wordpress/edit-widgets
package, and Lodash as a dependency altogether.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 #39495While this PR doesn't affect bundle size at all (because this package's code is not bundled), it still is a step forward in the direction of completely getting rid of Lodash in Gutenberg.
How?
We're replacing every Lodash call with native functionality.
Testing Instructions
ToolsMoreMenuGroup
still works well, register a few fills there (there are none by default).