fix: correct nested batch behavior. #218
Conversation
| }); | ||
|
|
||
| // nothing should be yet called | ||
| expect(child.mock.calls[2]).toEqual(undefined); |
There was a problem hiding this comment.
this mock would be called twice without the fix
|
Thanks! I guess this should be only relevant with React < 18. What do you think is the risk of such change now given that we are already updating to R18? |
theKashey
force-pushed
the
correct-nested-batch
branch
from
a800802
to
4b0c136
Compare
Why? Right now the real problem is the first scheduled update will be executed after every following update and React 18's ability to batch updates will not change this. Also, with some locations still using legacy Context API this change is the only opportunity to synchronise the mess - with one's ability to use |
|
Right, I see what you mean, missed the early return condition 👍 |
theKashey
force-pushed
the
correct-nested-batch
branch
from
8a7f08d
to
e89bef9
Compare
| } | ||
| // important to reset it before exiting this function | ||
| // as React will dump everything right after | ||
| batchedScheduled = false; |
There was a problem hiding this comment.
- all events will be executed in the "actually correct order" - First in, First out.
- all will be executed in one batch, mimicking React 18 behavior and reducing re-renders
- this can be a breaking change
There was a problem hiding this comment.
Let's maybe put this in a minor, but eager to test in Jira and see what breaks there 🙈
There was a problem hiding this comment.
It breaks one component doing read right after set in useEffect (QueryRenderer component). It's not clear how it was able to read the data also synchronously before.
Removing scheduleCallback fixes the problem and I think we can/need to remove it for React 18 auto-batching.
It's removal also might help with #219 as "flush" will be performed early.
|
|
||
| const executeBatched = () => { | ||
| unstable_batchedUpdates(() => { | ||
| while (batched.length) { |
There was a problem hiding this comment.
Could this be also written as:
let fn;
while ((fn = batched.shift())) {
fn();
}
Or am I missing the reason why we would replace batched with an empty array?
There was a problem hiding this comment.
I have a bad tendency for not using array modification methods as they are "slow".
But this way you can read and write it at the same time without "buffer" being reset.
Simplifies code a lot, 👍
| }); | ||
| batched.push(fn); | ||
|
|
||
| if (!batchedScheduled) { |
There was a problem hiding this comment.
Now that we handle batched inside the callback, do we need this check and batchedScheduled state?
If the batched queue is emptied automatically, then worst case is we call scheduleCallback and executeBatched does nothing as batched has been already flushed by the previous scheduleCallback 🤔
Just reasoning on the opportunity of removing extra statefulness.
There was a problem hiding this comment.
having batchedScheduled being implemented in a wrong way first time (note the comment before resetting it to false) - I am full in 🚀
| while (batched.length) { | ||
| const currentBatched = batched; | ||
| batched = []; | ||
| currentBatched.forEach((fn) => fn()); |
There was a problem hiding this comment.
This now looks a lot like the queue logic in schedule https://github.com/atlassian/react-sweet-state/blob/master/src/utils/schedule.js ... Should we strip that and handle everything inside batch? 🤔
There was a problem hiding this comment.
Sounds logical, batch can handle queue, but there will be a different in behavior
- now
scheduleexecuted all notifications at once - then
batchwill execute all notification and state updates in FIFO - for react it might not matter, we are in
batchedUpdates - but for callbacks it might matter, as before notification can arrive before state got updated, and now it will reach after
- need to find a way to write a test indicating this behavior change, if it exists
Solves #217