Different timing of updates between withSelect and useSelect

[michielvaneerd]

I'm not sure if this is a bug but I noticed that withSelect() and useSelect() call events/hooks at different times.

I have a block that shows the number of blocks. When I use withSelect() the number of blocks are updated immediately, but if I use useSelect() and delete a block with the mouse, then the update happens only after I click somewhere else in the post. See the two examples below:

My withSelect() code
(updates are immediately coming through)

const MyBlock = function(props) {
    return <div>Number of blocks: {props.count}</div>;
};

const MyBlockWithSelect = withSelect((select) => {
    return {
        count: select('core/block-editor').getBlockCount()
    };
})(MyBlock);

My useSelect() code
(delete a block with the mouse triggers no update, only after clicking somewhere else in the post)

const MyBlockUseSelect = function(props) {
    const {count} = useSelect((select) => {
        return {
            count: select('core/block-editor').getBlockCount()
        };
    }, []);
    return <div>Number of blocks: {count}</div>;
};

Is this intended behavior or am I missing something?

[youknowriad]

This sounds like a bug to me 🤔

cc @nerrad @epiqueras

[epiqueras]

Sounds like an async mode thing right?

withSelect had some odd undocumented behavior where it reran the selectors when async mode changed even if the store didn't update.

See #19007 (comment) and the following discussions.

[youknowriad]

It could be related to Async mode, and yes selectors should be run again when the Async mode changes.

[epiqueras]

I actually can't replicate this.

Try these blocks:

(function() {
	wp.blocks.registerBlockType("test/test-with-select", {
		title: "Test With Select",
		category: "common",
		edit: wp.data.withSelect(select => {
			return {
				blockCount: select("core/block-editor").getBlockCount()
			};
		})(function(props) {
			return props.blockCount;
		})
	});
	wp.blocks.registerBlockType("test/test-use-select", {
		title: "Test Use Select",
		category: "common",
		edit: function() {
			return wp.data.useSelect(
				select => select("core/block-editor").getBlockCount(),
				[]
			);
		}
	});
})();

They work as expected.

[aduth]

yes selectors should be run again when the Async mode changes.

Can you clarify why this is the expected behavior?

Edit: I guess so far as the original report, if this is necessary for those updates to be reflected, I can see why this is important. Maybe I'm misunderstanding the internals, but it seems to me there's some issue where a delayed update should be forced to run when the async value changes. That could be to the same effect as what you're suggesting?

[youknowriad]

@aduth My idea here is that if something was async and becomes sync, we can't be certain that the current props are fresh so we need to run the selectors again when the isAsync value changes. It might not be necessary for the other way (isAsync becoming false).

[epiqueras]

Well, if the props changed, there will be a queued update that will be flushed right?

[aduth]

Well, if the props changed, there will be a queued update that will be flushed right?

I'm wondering if there might be some issue with how this works or is expected to work. I mentioned similarly in #19205:

It's unclear to me whether there might be a related issue with the implementation of the priority queue where flushes should be expected to force the onStoreChange callback (see #13056 (comment)).

When useSelect "flushes" a changing isAsync, there's nothing about this which guarantees that onStoreChange would be run, because as noted at #13056 (comment), the flush doesn't actually invoke a callback, it only removes it from the waiting list.

#19205 is effective at least in ensuring that when isAsync changes, onStoreChange will be called (possibly at next idle availability, based on the new value of isAsync). But I wasn't sure if it would be equally effective to change the behavior of flush to call the callback. Some initial explorations here weren't too promising.

[epiqueras]

Ah, yes that would break things. It should have been called .clear instead of .flush.

I think it's best to make it actually flush and that should fix things. Then we can just add a dev note that says that useSelect and withSelect don't re-run selectors when the only thing that changed is the async mode, which shouldn't really affect anyone.

[aduth]

See also #19282 for priority queue implementation.

Some initial explorations here weren't too promising.

Unfortunately, I'm still not seeing any immediate benefit from #19282.

[epiqueras]

Seems to work?

https://youtu.be/nZmTk4tIZLo

[aduth]

This is what I see, both on master and in the update/priority-queue-flush branch (#19282):

https://cloudup.com/cAdBE6KTvYU

It updates both consistently when using the changes from update/use-select-async-invalidate (#19205).

[epiqueras]

OK, I've reproduced this. You need the selection to jump to the useSelect test block after deleting the third block for this issue to surface.

This is actually not being caused by not re-running selectors when async mode changes.

Here is what happens:

• Block count updates.
• Subscription fires while block is still in async mode.
• The queue context changes, because useMemo is rarely respected:

You may rely on useMemo as a performance optimization, not as a semantic guarantee. In the future, React may choose to “forget” some previously memoized values and recalculate them on next render, e.g. to free memory for offscreen components. Write your code so that it still works without useMemo — and then add it to optimize performance.

• The queued callback is lost so we have to wait for another update.

#19205 only fixes this by introducing an extra update that should be unnecessary.

The correct way to fix this is by using a stable-cache-useMemo. We also need #19282 merged so that if the async mode changes before the queue is cleared, it still flushes correctly.

[epiqueras]

Another issue is that when async mode changes, we flush the queue before running latestMapOutput.current = mapOutput;, so changes to mapOutput by the flushed callback are overwritten.

PR incoming.

[epiqueras]

Fixed: #19286.