Add suspense support to the data module #37261
Conversation
youknowriad
added
[Type] Technical Prototype
| * | ||
| * @return {Function} A custom react hook. | ||
| */ | ||
| export function useSuspenseSelect( mapSelect, deps ) { |
There was a problem hiding this comment.
This is basically the exact same code as useSelect, the only differences are the places where we throw shouldBeSuspended
There was a problem hiding this comment.
I know it's a draft so at the risk of stating the obvious: It would be nice to reduce the duplication somehow. Maybe an option like isSuspense, or maybe a common abstraction?
There was a problem hiding this comment.
What would be cool would be to automatically fallback to non-suspense version if the Suspense provider is detected (or opposite thought not sure about auto-suspending everything :P )
|
Size Change: +325 B (0%) Total Size: 1.24 MB
ℹ️ View Unchanged
|
| fallback={ | ||
| <h1> | ||
| Very Ugly | ||
| Loading State |
|
I watched the talk from React Conf 2021 and it looks like there is a new API dedicated to external store libraries that resolves the tearing issue that can happen in concurrent rendering:
All the details can be found in the talk from Daishi Kato: https://www.youtube.com/watch?v=oPfSC5bQPR8 I guess we already have the controlled tearing issue when using async updates for parts of the UI that are less important. Anyway, it looks like there are going a few moving parts to take into account. |
React Query in suspense mode throws promises, too. I think the promise-throwing is going to stay, as it's a rather fundamental aspect of suspense. |
There was a problem hiding this comment.
There are two ways how useSuspenseSelect can behave, with subtle differences. After calling this:
const c = useSuspenseSelect( ( select ) => {
const a = select( storeA ).get();
const b = select( storeB ).get();
return op( a, b );
} );The current implementation will get() from both stores, with a potentially being an unresolved null, passes that null to op, and only after the callback finishes, proceeds to ignore the result and throw/suspend.
An alternative, imo slightly better impl would be to throw right out of the select( storeA ).get() call, never proceeding beyond that unresolved line. The author of the callback has a guarantee that select( storeA ).get() always returns a resolved value, not some null, and avoid a lot of null checks.
Another advantage of the alternative is that the "useSuspenseSelect operation is distributive", which is a fancy way of saying that:
const a = useSuspenseSelect( ( select ) => select( storeA ).get() );
const b = useSuspenseSelect( ( select ) => select( storeB ).get() );
const c = op( a, b );Does this behave exactly the same way as the single unified useSuspenseSelect call above? One implementation guarantees that, the other doesn't.
After all, it's quite common in Gutenberg to select something from one store, like some entity kind or ID, and then feed that value into another select. But if the first select is unresolved, the second select doesn't even know what to wait for?
| const state = store.getState(); | ||
| if ( resolversCache.isRunning( selectorName, args ) ) { | ||
| registry.__unstableSuspend( | ||
| registry.resolveSelect( |
There was a problem hiding this comment.
This looks like potential infinite recursion? Calling a selector starts to resolve it, calling the resolver, then the resolver calls the selector again, triggering another resolution again? Maybe it's all carefully orchestrated so that the recursion always ends quickly, just flagging this as something that's worth a thorough review and testing.
|
@jsnajdr Good point that said, I'm not sure yet about the perf implication here.
I think this is true but since suspense is not supported yet, you could argue that the fallback is probably already handled anyway. It can be a nice way for the future to always assume that these things are resolved but is it a good thing? From a DevX perspective, it's definitely a good thing but I fear the waterfall effect if consecutive calls don't share any dependency. I think this is also probably something common. So there are downsides for both 🤷 |
It's not only a good thing, but it's exactly how suspense code should be written. When writing a suspense component, you call data-fetching functions while pretending that everything is synchronous and assuming that the fetching function always returns the data immediately: function Post( { postId } ) {
const post = fetchPost( postId );
return <div><h1>{ post.title }</h1><p>{ post.excerpt }</p></div>;
}There's no checking If the So, it only makes sense that the |
youknowriad
force-pushed
the
add/suspense-support
branch
from
d69b9a2 to
2715a8d
Compare
jsnajdr
force-pushed
the
add/suspense-support
branch
from
2715a8d to
325d58b
Compare
|
Did a rebase onto latest |
jsnajdr
force-pushed
the
add/suspense-support
branch
from
1906125 to
52d0104
Compare
|
@youknowriad I reimplemented the useSuspenseSelect( ( select ) => {
const id = select( 'store' ).getId();
const post = select( 'store' ).getPost( id );
const author = select( 'store' ).getAuthor( post.author );
return { author };
}, [] );Here each of the selectors, even though it's async, guarantees to always return a valid I left in place your experimental usages, like the "Very Ugly Loading State" placeholder in Site Editor. You can use the hook and the It's not possible to detect in the But I don't think it's a good idea: the difference between What I'll continue to work on now is a unit test suite that tests various scenarios. And I also want to modify behavior a bit in case when resolution fails for some selectors. In that case the correct thing to do is to read the resolution error and throw it. I.e., |
|
Thanks for the update.
The reasoning makes sense, I just want us to avoid breaking changes though for folks as we introduce "suspense" usage in blocks. We need to add the provider for our core block editors and maybe figure out a way to add it automatically to third-party block editors as well |
What could be good news is that you can safely move from With |
Done. There is a suite of unit tests, and I indeed had to modify the behavior on resolution failure in order to trigger an error boundary in one of the tests. The |
jsnajdr
force-pushed
the
add/suspense-support
branch
from
5893183 to
bc2ba99
Compare
jsnajdr
force-pushed
the
add/suspense-support
branch
from
bc2ba99 to
8107b90
Compare
|
I rebased this PR onto the latest It's unfortunate that there's a lot of copy&paste from |
| } | ||
|
|
||
| return mapOutput; | ||
| } |
There was a problem hiding this comment.
How different is the code here from the "raw" useSelect? Asking to see if we can share the same hook (or at least same base implementation) or if it's completely different?
There was a problem hiding this comment.
Nevermind, I just saw your comment above :P
There was a problem hiding this comment.
The biggest difference, not easy to wrap in some if ( suspenseMode ) conditions, is how errors thrown by mapSelect are handled. useSelect kind of doesn't know what to do with them, it logs them to console, and otherwise tries to ignore them: it returns the previous (successful) mapSelect result, and hopes maybe on the next call the error will go away.
useSuspenseSelect simply rethrows the error, and lets suspense or error boundary to catch it.
|
What would be the plan for the next steps to implement this in site editor, patterns previews and such? Would be cool to improve the loading state for these. |
I don't know, I thought the plan was that you will try to implement the loading states 🙂 |
haha :) that's fine. I think I won't be able to get to it soon but we'll see. |
|
I can't approve my own PR, so feel free to approve and merge :) |
jsnajdr
force-pushed
the
add/suspense-support
branch
from
8107b90 to
f25f226
Compare
jsnajdr
force-pushed
the
add/suspense-support
branch
from
f25f226 to
23ecd56
Compare
|
It looks like this was never added to the |
mburridge
added
the
Needs Dev Note
|
Added Needs Dev Note label in case a dev note is needed for 6.1 release. |
Be prepared for some ugly code, bug ugly code for pocs is always the best kind of code.
Anyway, the idea here is simple, instead of doing
useSelect, we should be able to douseSuspenseSelectin order to have the exact same result but for any thing that is async (in our case, it means for anything that has a resolver), suspend the rendering and tell react that it's not ready yet.In other words, add Suspense support to the data module.
What this allows us to do?
Right now, when loading the editor (or more visibility in the site editor), there's a waterfall effect, the template is loaded and then rendered with a bunch of spinners and then the template parts get loaded and then site logo shows a spinner, same for query loops... until everything is loaded.
With suspense we can declaratively say, show this fallback loading state until everything is ready pretty easily. You can try the site editor in this PR to see how it compared to trunk.
There are still some glitches right now that I haven't been able to pinpoint yet. Basically when the template parts loads, there's a very small delay of some milliseconds that pass before the site logo block get rendered suspending the rendering once more, this ends up with a glitch showing the loading state twice separated with a very small delay where the actual components in their temporary state are rendered. I think this should be solvable but we need to figure out where the asyncness is happening to be able to do that.
Open questions