useSelect: incrementally subscribe to stores when first selected from #47243
Conversation
|
Size Change: +61 B (0%) Total Size: 1.34 MB
ℹ️ View Unchanged
|
jsnajdr
force-pushed
the
update/use-select-resubscribe
branch
from
7b1e9e5 to
e230e05
Compare
|
This goes beyond my understanding of the internals of |
jsnajdr
force-pushed
the
update/use-select-resubscribe
branch
from
e230e05 to
3455787
Compare
|
Flaky tests detected in 490e315. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4403530658
|
|
Thanks for the thorough explanation in the PR, @jsnajdr 👍 There's a bunch of test failures - any chance they could be related? |
Yes, it's very likely that there's something wrong with the new |
jsnajdr
force-pushed
the
update/use-select-resubscribe
branch
from
3455787 to
222514a
Compare
|
I'm returning back to this after some time. A mobile unit test is failing where a Button block is created and saved. The |
|
Something goes wrong when there are two selects where the second depends on the result of the first one: const first = useSelect( select => select( store ).getFirst() );
const second = useSelect( select => first + select( store ).getSecond(), [ first ] );Then, after the Before this PR, the second select would re-subscribe to the |
jsnajdr
force-pushed
the
update/use-select-resubscribe
branch
2 times, most recently
from
cf782c4 to
490e315
Compare
|
Well this was quite hard to debug but I think I found the culprit. When there is a useSelect( ( select ) => {
return select( blockEditorStore ).getBlockName( clientId );
}, [ clientId ] );we need to correctly handle the transition when the The following was happening here, leading to bad behavior of the editor (crashes, failed tests):
So, it can happen that a render with a new My solution is to set the Before this PR, we were always re-subscribing to all stores on a dependency update. New subscription invalidates the cached value, so the bug was never triggered. Now, after this PR, the code is smarter, it knows that we still read only from At this moment I don't know how to write a test to catch the bug. I suspect that in |
|
This PR is now green and can be reviewed and merged. FYI @youknowriad @kevin940726 |
|
I have some concerns whether this const name = useSelect( ( select ) => {
return select( blockEditorStore ).getBlockName( clientId );
}, [ clientId ] );The component with the hook is mounted, with
Before this PR, I don't have proof or test that this really happens, and I didn't see it happen in production. But the above reasoning tells me that it can happen. |
|
There is a |
jsnajdr
force-pushed
the
update/use-select-resubscribe
branch
from
490e315 to
1bee787
Compare
I looked at this more closely today, and tested both our and I improved the test cases for this. There was an old test for updates between render and subscribe, and I simplified it, removing class component in favor of a functional one. And added a new test case for the store update between selector change and effect. Both test cases schedule the insidious store update using This PR is ready for review and merging once again. |
|
@youknowriad @kevin940726 Could you please have a look at this one? Seems like the pings went through the cracks. |
| @@ -109,7 +122,7 @@ describe( 'useSelect', () => { | |||
| ); | |||
|
|
|||
| expect( selectSpyFoo ).toHaveBeenCalledTimes( 2 ); | |||
| expect( selectSpyBar ).toHaveBeenCalledTimes( 2 ); | |||
| expect( selectSpyBar ).toHaveBeenCalledTimes( 1 ); | |||
There was a problem hiding this comment.
So there's a perf improvement here or something?
There was a problem hiding this comment.
Kind of, because when you do a select like this:
const name = useSelect( ( select ) => {
return select( blockEditorStore ).getBlockName( clientId );
}, [ clientId ] );then before this PR, when the clientId dependency changed, useSelect unsubscribed from the blockEditorStore and then immediately subscribed again. After this PR, this is no longer done, because useSelect knows that we're still selecting from this one store, that the set of observed stores didn't change. Only the clientId param has changed.
This kind of select is very common in block sidebar and toolbar, as they show info for the currently selected block.
ryanwelcher
added
the
[Type] Bug
|
I actually suspect that this PR increased typing performance by approximatively 10%. Is there anything we can do about it. If we can't do anything about it, we should ask ourselves what's better:
|
|
It actually impacted other metrics similarly as well (block select, inserter open, inserter hover) |
The extra work the new version does is tracking the
Now, to support conditional selects, we need to check on each store update.
One thing that could improve the perf again is getting rid of
That's not an easy tradeoff because we'd be trading performance for correctness. Like the Pentium fdiv bug from 1994 🙂 |
Indeed, because we never supported these conditional selects, personally I'm willing to just document that we don't support them. I don't think it's incorrect, it's just different. |
Fixes #47145. On every call of
mapSelect, it looks at the list of stores it selected from and incrementally subscribes to new ones.A typical lifecycle in a React component looks like this:
useSelecthook, anduseSyncExternalStoreinside it calls thegetValuefunction. ThegetValuefunction will callmapSelect, and initialize theactiveStoresvariable with the list of selected stores.useSyncExternalStorecalls thesubscribefunction. It subscribes to theactiveStores.useSelectcallsmapSelectagain, and notes which stores were selected from this time. It callsupdateStoreswith this fresh list.updateStoreswill add new stores to theactiveStoreslist, so that newsubscribecalls subscribe to them, too. And will also go through list ofactiveSubscriptionsand subscribes to the new stores for each of them.useSelecthook unmounts and all the subscribed stores are unsubscribed from.The
updateStorescall is done duringgetValue, which is called during render, which is not 100% OK, because it's a side-effect. But we don't have any other opportunity to update the subscriptions.To make
updateStoressafe, I'm only adding new subscriptions, never removing old ones. If one call ofmapSelectselects fromstore-1and a later call selects fromstore-2, we keep the subscription tostore-1, and only add a new subscription tostore-2. This is more concurrent-mode safe. It could happen that auseSelectrender would unsubscribe fromstore-1and then the results of that render wouldn't be committed. Because of a higher-priority update, because of suspension, because of error. The subscription tostore-1would be incorrectly removed.Cc: @davilera
How to test: Covered by unit tests.