Lodash: Remove _.get() from various blocks
#48491
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tyxla
added
[Type] Performance
tyxla
requested review from
Mamaduka,
sgomes,
aristath,
jsnajdr,
gziolo,
swissspidy,
ellatrix,
flootr,
chad1008,
ntsekouras,
fabiankaegy and
brookewp
|
Size Change: +142 B (0%) Total Size: 1.34 MB
ℹ️ View Unchanged
|
tyxla
commented
Feb 27, 2023
| @@ -247,8 +246,8 @@ function Placeholder( { clientId, name, setAttributes } ) { | |||
| return ( | |||
| <div { ...blockProps }> | |||
| <__experimentalBlockVariationPicker | |||
| icon={ get( blockType, [ 'icon', 'src' ] ) } | |||
| label={ get( blockType, [ 'title' ] ) } | |||
| icon={ blockType?.icon?.src } | |||
There was a problem hiding this comment.
blockType could be undefined:
| @@ -57,7 +56,7 @@ const USERS_LIST_QUERY = { | |||
| }; | |||
|
|
|||
| function getFeaturedImageDetails( post, size ) { | |||
| const image = get( post, [ '_embedded', 'wp:featuredmedia', '0' ] ); | |||
| const image = post._embedded?.[ 'wp:featuredmedia' ]?.[ '0' ]; | |||
There was a problem hiding this comment.
post is certainly defined an an object, and used as one earlier:
The rest may not exist, so we're adding optional chaining.
| 0 | ||
| ), | ||
| defaultImageWidth: | ||
| settings.imageDimensions?.[ featuredImageSizeSlug ] |
There was a problem hiding this comment.
settings will always be an object:
but since it can be modified, we can't be certain that imageDimensions will exist. As a matter of fact, it doesn't exist in the default settings. So we're adding optional chaining for the rest of the chain, and nullish coalescing for the default value.
| [] | ||
| ); | ||
| const colors = | ||
| select( blockEditorStore ).getSettings().colors ?? []; |
There was a problem hiding this comment.
Same as above, getSettings() will always return an object, but colors may not exists, in which case we'll default to the empty array.
Comment on lines
+69
to
+76
| const getValueFromObjectPath = ( object, path ) => { | ||
| const normalizedPath = path.split( '.' ); | ||
| let value = object; | ||
| normalizedPath.forEach( ( fieldName ) => { | ||
| value = value?.[ fieldName ]; | ||
| } ); | ||
| return value; | ||
| }; |
There was a problem hiding this comment.
We're introducing a simple helper here to separate the concerns. It's as simple as possible: supporting only a string path, separated by dots (which was the original behavior).
There was a problem hiding this comment.
Just wondering if we could use such a helper in other places, too? It seems worth to follow up and/or add a note so we don't forget about it. I know it's just a tiny little helper function but tests may be worthwhile (leaving that to you).
There was a problem hiding this comment.
I don't think it's worth the effort and packaging, to be honest. Also, one of the benefits of Lodash has been decoupling and IMHO such a simple helper won't bring many benefits for the added cost of additional dependencies and additional coupling a utility package might bring over time.
There was a problem hiding this comment.
I would love for this to be tested. It seems there aren't a lot of tests for anything in this file though.
There was a problem hiding this comment.
Happy to add some tests in a follow-up PR. Will ping you for taking a look.
| columnCount === undefined | ||
| ? get( firstRow, [ 'cells', 'length' ] ) | ||
| : columnCount; | ||
| columnCount === undefined ? firstRow?.cells?.length : columnCount; |
There was a problem hiding this comment.
Theoretically, firstRow could be undefined, thus the optional chaining.
| {} | ||
| ); | ||
| const firstCellInColumn = | ||
| firstRow?.cells?.[ index ] ?? {}; |
There was a problem hiding this comment.
Theoretically, firstRow could be undefined, thus the optional chaining.
| @@ -310,7 +301,7 @@ export function toggleSection( state, sectionName ) { | |||
| } | |||
|
|
|||
| // Get the length of the first row of the body to use when creating the header. | |||
| const columnCount = get( state, [ 'body', 0, 'cells', 'length' ], 1 ); | |||
| const columnCount = state.body?.[ 0 ]?.cells?.length ?? 1; | |||
There was a problem hiding this comment.
body should always exist as per my manual checks, but I prefer not taking chances.
| @@ -64,7 +59,7 @@ export default function TextColumnsEdit( { attributes, setAttributes } ) { | |||
| > | |||
| <RichText | |||
| tagName="p" | |||
| value={ get( content, [ index, 'children' ] ) } | |||
| value={ content?.[ index ]?.children } | |||
There was a problem hiding this comment.
We're being flexible here since the content comes from an outside attribute and could potentially not exist.
| @@ -20,7 +15,7 @@ export default function save( { attributes } ) { | |||
| <div className="wp-block-column" key={ `column-${ index }` }> | |||
| <RichText.Content | |||
| tagName="p" | |||
| value={ get( content, [ index, 'children' ] ) } | |||
| value={ content?.[ index ]?.children } | |||
There was a problem hiding this comment.
We're being flexible here since the content comes from an outside attribute and could potentially not exist.
|
Flaky tests detected in 57e1add. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4373592325
|
flootr
reviewed
Feb 28, 2023
| 'large', | ||
| 'source_url', | ||
| ] ); | ||
| media?.sizes?.large?.url || |
There was a problem hiding this comment.
Seems like media should be defined. Shall we remove the first ??
There was a problem hiding this comment.
Good call 👍
Removed in dc1ab15. ✅
I also removed the second one, since this is wrapped in an if() that ensures that media.sizes will also be defined.
Comment on lines
+69
to
+76
| const getValueFromObjectPath = ( object, path ) => { | ||
| const normalizedPath = path.split( '.' ); | ||
| let value = object; | ||
| normalizedPath.forEach( ( fieldName ) => { | ||
| value = value?.[ fieldName ]; | ||
| } ); | ||
| return value; | ||
| }; |
There was a problem hiding this comment.
Just wondering if we could use such a helper in other places, too? It seems worth to follow up and/or add a note so we don't forget about it. I know it's just a tiny little helper function but tests may be worthwhile (leaving that to you).
|
@flootr I'd appreciate another review of this PR when you get a chance. |
flootr
approved these changes
Mar 9, 2023
Comment on lines
+69
to
+76
| const getValueFromObjectPath = ( object, path ) => { | ||
| const normalizedPath = path.split( '.' ); | ||
| let value = object; | ||
| normalizedPath.forEach( ( fieldName ) => { | ||
| value = value?.[ fieldName ]; | ||
| } ); | ||
| return value; | ||
| }; |
There was a problem hiding this comment.
I would love for this to be tested. It seems there aren't a lot of tests for anything in this file though.
tyxla
force-pushed
the
remove/lodash-get-remaining-blocks
branch
from
dc1ab15
to
57e1add
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
This PR removes Lodash's
_.get()from various blocks that are using it.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 using optional chaining and nullish coalescing instead.
Testing Instructions
largeorfullsize.mainColorattribute still properly displays the border colors.