Add regression test for #18497

[sophiebits]

No description provided.

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 23ae5d4:

Sandbox	Source
charming-gauss-xpjpu	Configuration

[sizebot]

No significant bundle size changes to report.

Size changes (experimental)

Generated by 🚫 dangerJS against 23ae5d4

[sizebot]

No significant bundle size changes to report.

Size changes (stable)

Generated by 🚫 dangerJS against 23ae5d4

[sophiebits]

It looks like this logic is to blame:

react/packages/react-reconciler/src/ReactFiberWorkLoop.js

Lines 357 to 365 in 2dddd1e

	// If we're in the middle of rendering a tree, do not update at the same
	// expiration time that is already rendering.
	// TODO: We shouldn't have to do this if the update is on a different root.
	// Refactor computeExpirationForFiber + scheduleUpdate so we have access to
	// the root when we check for this condition.
	if (workInProgressRoot !== null && expirationTime === renderExpirationTime) {
	// This is a trick to move this update into a separate batch
	expirationTime -= 1;
	}

It applies to the "2" update but not "3", causing the "2" update to have a lower priority than the "3" update, which is why we render with only [1,3].

[gaearon]

Seems like you can work around it by making it discrete: https://codesandbox.io/s/agitated-ramanujan-hwxjg?file=/src/App.js

Seb says this particular case will likely go away after a planned big refactor Andrew is working on.

But also that we don't make very strong guarantees about which updates get skipped. We only make guarantees about terminal value (eventual consistency) and that discrete updates would flush in order (like in my linked demo).

[sophiebits]

My understanding of discrete is you effectively lose the concurrency support. The only reason I started using concurrent mode is that I wanted React to automatically batch the renders (more than one mouse move per render). Changing this to be discrete feels like a little bit of a waste.

So it isn't a guarantee that updates at the same priority are processed in order? That's completely contrary to all my past understanding of how concurrent mode works. I thought we subset only based on a priority threshold.

[sophiebits]

(I thought discrete is only meant for if you need it to be flushed before the next event handler runs.)

[gaearon]

I thought we subset only based on a priority threshold.

That’s what I thought too but I’m hearing we are relaxing some of the previous constraints. There are cases where the current model has very unintuitive behavior, and Andrew is working on an alternative one that splits updates into “threads” that aren’t directly tied to priority levels. I haven’t looked in depth at these changes yet so it’s hard for me to summarise them or how exactly the thinking has evolved. Maybe we can ask Seb to do this.

Generally in the past few months we’ve been mostly focused on solving cases that we see come up in practical product usage. So I think that shifted the focus a bit from the purely theoretical perspective when we didn’t really know which constraints mattered and which would create other issues.

I’m curious to learn more about how you ran into this one and what the product case that exhibits the bug looked like.

[sophiebits]

Drawing app. Add a new point every mousemove. This bug means that sometimes you are drawing:

And then it adds points in the past and the line jumps around:

[sophiebits]

@gaearon Worth committing a regression test?

[gaearon]

I think this makes sense. If Andrew disagrees he can revert but IMO it’s helpful coverage even if we decide to change these semantics.

[gaearon]

#19215