useGlobalStylesOutput: Use memo for derived values #42917
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
|
@andrewserong, @jorgefilipecosta do you mind having a look at this PR? I think you've worked with this hook before. Edit: Tagged different Jorge 😅 |
Mamaduka
added
[Type] Code Quality
|
Size Change: -41 B (0%) Total Size: 1.27 MB
ℹ️ View Unchanged
|
andrewserong
reviewed
Aug 4, 2022
There was a problem hiding this comment.
Thanks for taking a look at this @Mamaduka! This is testing nicely for me across a couple of real-world block-based themes and emptytheme. I tested making changes to root level global styles, colors and typography, block level styles, element styles, and the theme style variations render correctly.
Conceptually I think a useMemo is a better fit than the useState / useEffect combo, too 👍
Just left a comment about the default return value — I wasn't sure if we need to preserve the first return types for compatibility? I couldn't find any issues with the empty array in practice in my testing, though.
| if ( ! mergedConfig?.styles || ! mergedConfig?.settings ) { | ||
| return; | ||
| return []; |
There was a problem hiding this comment.
Does this return statement mean that the whole function could early return an empty array?
It looks like prior to this PR, the function will always return an array with four elements. From looking at the useState calls, an equivalent to the default values in that case might be something like:
return [ [], {}, {}, true ] ?
There was a problem hiding this comment.
Does this return statement mean that the whole function could early return an empty array?
Correct. Initially, I was planning to return the array with the same signature, but then I checked the usage, and returning undefined for values won't be an issue.
There was a problem hiding this comment.
but then I checked the usage, and returning undefined for values won't be an issue.
Very interesting! Particularly because of hasBlockGapSupport having a default truthy value. Given that the function doesn't appear to be exported outside of the edit-site package, this sounds okay to me in principle.
One thing to keep in mind with making changes to the output, is that from the documentation it sounds like there might have been (or be) an intention of potentially extracting the global styles APIs to their own package (this line). I think I remember @jorgefilipecosta working on making global styles data available in the post editor a while back, but I'm not sure if that's related.
I see that including hasBlockGapSupport was added to the output of the function in #40185 in April. From the looks of it, that value was only needed in toStyles and doesn't appear to be used by any consumers of useGlobalStylesOutput. It might be worth double-checking with @oandregal to see if there was a reason it was added to the output or whether it's still needed? It looks like we might be able to remove it altogether from the return value 🤔
There was a problem hiding this comment.
I think this is the best time to change return types if we want 😄
I was wondering about the hasBlockGapSupport return as well. So let'ss see if we can remove it.
There was a problem hiding this comment.
I see that including hasBlockGapSupport was added to the output of the function in #40185 in April. From the looks of it, that value was only needed in toStyles and doesn't appear to be used by any consumers of useGlobalStylesOutput.
Yeah, I don't see it in use anywhere either and don't remember why it was returned in the first place :)
There was a problem hiding this comment.
Thanks for the confirmation, @oandregal. I removed hasBlockGapSupport from the return value.
andrewserong
approved these changes
Aug 4, 2022
There was a problem hiding this comment.
This is testing nicely for me, and as you mention, the existing usage doesn't appear to be affected by returning early with an empty array. Overall, I prefer this useMemo approach to what we had previously 👍
It might be worth getting a second review, too, from folks who were more involved in the API design of global styles to confidence check the signature change for the initial default value — but I might be being overly cautious!
|
Thanks for testing and detailed review, @andrewserong 🙇 |
Mamaduka
force-pushed
the
try/global-styles-output-use-memo
branch
from
37406ec to
26b7d72
Compare
|
@andrewserong, I'm going to merge this once all checks are green. Happy to address any follow-up feedback. |
|
Excellent, thanks for landing this one @Mamaduka, and good to confirm that we didn't need |
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?
PR updates
useGlobalStylesOutputto useuseMemofor calculations instead ofuseEffectand state.Why?
I think it makes more sense to use memoization in this context. Bonus: It removes extra rendered triggered by state updates.
Testing Instructions
Confirm that global styles work as before on the Site Editor. I tested with multiple block-based themes, including the "Empty Theme"