Patterns: avoid fetching on load #57999
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
|
Size Change: -5.63 kB (0%) Total Size: 1.69 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 7bdf71f. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7639313281
|
ellatrix
force-pushed
the
try/avoid-patterns-fetching-on-load-3
branch
from
cf64f9c
to
68f3515
Compare
ellatrix
commented
Jan 19, 2024
| @@ -4205,382 +4201,6 @@ describe( 'selectors', () => { | |||
| } ); | |||
| } ); | |||
|
|
|||
| describe( '__experimentalGetAllowedPatterns', () => { | |||
There was a problem hiding this comment.
These tests are moved to a separate file (above), because they now depend on a registered store.
ellatrix
commented
Jan 22, 2024
| @@ -0,0 +1,6 @@ | |||
| export const getFetchedPatterns = | |||
There was a problem hiding this comment.
Note that this resolves a private selector.
ellatrix
commented
Jan 22, 2024
| ( state ) => [ getAllPatterns( state ) ] | ||
| ); | ||
|
|
||
| const getAllAllowedPatterns = createSelector( |
There was a problem hiding this comment.
Was not exposed, so absorbed it into __experimentalGetAllowedPatterns.
ellatrix
commented
Jan 22, 2024
| */ | ||
| import { INSERTER_PATTERN_TYPES } from '../components/inserter/block-patterns-tab/utils'; | ||
|
|
||
| export const getUserPatterns = createSelector( |
There was a problem hiding this comment.
I absorbed this into the getAllPatterns private selector.
ellatrix
force-pushed
the
try/avoid-patterns-fetching-on-load-3
branch
from
0a77097
to
e559aab
Compare
youknowriad
reviewed
Jan 23, 2024
| @@ -0,0 +1,6 @@ | |||
| export const getFetchedPatterns = | |||
| ( fetch ) => | |||
There was a problem hiding this comment.
If I'm not wrong, "fetch" comes from the store itself, so why not select it instead?
There was a problem hiding this comment.
On a related note, what happens when the __exFetchBlockPatterns setting changes? Do we detect that and invalidate/refetch the resolved selector state, or ignore the change and resolve only once, with the "fetch" function that was present at the time?
There was a problem hiding this comment.
It was a remnant from old code, will adjust it
There was a problem hiding this comment.
Adjusted :) I also added shouldInvalidate.
jsnajdr
reviewed
Jan 23, 2024
| __experimentalBlockPatterns, | ||
| __experimentalFetchBlockPatterns, | ||
| __experimentalUserPatternCategories = [], | ||
| __experimentalReusableBlocks = [], |
There was a problem hiding this comment.
Note that __exUserPatternCategories, __exReusableBlocks and __exBlockPatternCategories are exactly the same kind of setting as __exBlockPatterns: their true source is also an async REST endpoint, and they are fetched prematurely on editor load, in the block editor provider component.
We should also migrate them to this new approach with a "fetch" function in settings. It's not a blocker for this PR, but something we should think about.
How will we have to change getAllPatterns and other selectors when there is also a getFetchedX selector+resolver also for the three other entities? Will it be a straightforward change?
There was a problem hiding this comment.
That is exactly the plan 🙂
There was a problem hiding this comment.
And yes the others should be more straightforward
jsnajdr
reviewed
Jan 23, 2024
| ...__experimentalBlockPatterns, | ||
| ...unlock( select( store ) ).getFetchedPatterns( | ||
| __experimentalFetchBlockPatterns | ||
| ), |
There was a problem hiding this comment.
Maybe, instead of merging __exBlockPatterns and __experimentalFetchBlockPatterns, we should treat __exBlockPatterns as a legacy fallback: use it only when __experimentalFetchBlockPatterns is not specified at all. Then __exBlockPatterns feels more like a deprecated setting rather than one of two supported alternatives.
There was a problem hiding this comment.
I changed this back to the original because there's still a use case for loading patterns locally through the settings without it triggering a re-fetch from the server. We can polish this later I guess if we would still like to unify it.
| @@ -0,0 +1,6 @@ | |||
| export const getFetchedPatterns = | |||
| ( fetch ) => | |||
There was a problem hiding this comment.
On a related note, what happens when the __exFetchBlockPatterns setting changes? Do we detect that and invalidate/refetch the resolved selector state, or ignore the change and resolve only once, with the "fetch" function that was present at the time?
| } | ||
| return { | ||
| ...pattern, | ||
| blocks: parse( pattern.content, { |
There was a problem hiding this comment.
This is another thing to think about: with lazy block loading, the parse function becomes async and __exGetParsedPattern must become a selector with resolver and associated reducer state. And all the callers, like _exGetAllowedPatterns, must be modified accordingly. I had it working in the #51778, but it deteriorated over time as pattern code in trunk got modified. And this PR will force another update.
There was a problem hiding this comment.
Is it a blocker for this PR? Should I adjust anything?
There was a problem hiding this comment.
Not a blocker, just something to consider -- that we'll probably want to evolve in that direction in near future. Is this PR going in the same direction (making that change easier) or in the opposite direction (making it harder)?
There was a problem hiding this comment.
Not sure it makes it easier or harder, it's probably the same as before. The only difference is that you can now take advantage of resolvers, so that might help.
jsnajdr
reviewed
Jan 23, 2024
| const blockPatterns = __experimentalFetchBlockPatterns | ||
| ? unlock( select( store ) ).getFetchedPatterns() | ||
| : __experimentalBlockPatterns; | ||
| return [ ...userPatterns, ...blockPatterns ]; |
There was a problem hiding this comment.
The blockPatterns variable will be undefined when neither setting is specified, and then the array spread will crash. We'll need to default __experimentalFetchBlockPatterns to [] or something similar.
There was a problem hiding this comment.
Defaulted __experimentalBlockPatterns. Also changed getFetchedPatterns to always return an array.
jsnajdr
reviewed
Jan 23, 2024
| const restPatterns = await apiFetch( { | ||
| path: '/wp/v2/block-patterns/patterns', | ||
| } ); | ||
| return restPatterns?.map( ( pattern ) => |
There was a problem hiding this comment.
This ?. optional chaining, if restPatterns is ever truly null-ish, leads to fetchBlockPatterns returning null, and that value will then sneak into Redux state, which expects array. Let's make sure the return value is always an array.
ellatrix
force-pushed
the
try/avoid-patterns-fetching-on-load-3
branch
from
a497f0d
to
43522b7
Compare
ellatrix
force-pushed
the
try/avoid-patterns-fetching-on-load-3
branch
from
43522b7
to
cb49561
Compare
|
Thanks for the review @jsnajdr! |
7 tasks
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? Why?
Currently all patterns are fetched from the server at editor setup, to be passed to the block editor as a setting. This is not ideal because the editor doesn't require the list of patterns until you open the inserter.
This PR adds a new setting that is an async function to get the patterns. This function can then be used in a resolver in the blocks-editor store, allowing us to fetch the patterns only when needed.
The PR adds a
__experimentalFetchBlockPatternsblock editor setting (async function), while retaining the__experimentalBlockPatternssetting (array).How?
One interesting implementation detail is that resolvers cannot be called from plain state selectors. They must be called within a registry selector. The current patterns selectors are memoized selectors. Thanks to #57888 and #57943 this works now!
Testing Instructions
Open the inserter's patterns tab. All e2e tests should pass.
Testing Instructions for Keyboard
Screenshots or screencast