Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: correct nested batch behavior. #218

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

fix: correct nested batch behavior. #218

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4b0c136
fix: correct nested batch behavior. Solves #217
theKashey Nov 17, 2023
e89bef9
revamp batching implementation to batch in longer sequences
theKashey Dec 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions src/utils/__tests__/batched-updates.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,13 +56,20 @@ describe('batch', () => {
const child = jest.fn().mockReturnValue(null);
render(<TestComponent>{child}</TestComponent>);
const update = child.mock.calls[0][0];
act(() => update());
act(() => {
update();
update();
update();
});

// nothing should be yet called
expect(child.mock.calls[2]).toEqual(undefined);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this mock would be called twice without the fix


// scheduler uses timeouts on non-browser envs
await act(() => new Promise((r) => setTimeout(r, 10)));

// assertion no longer relevant with React 18+
expect(child.mock.calls[2]).toEqual([expect.any(Function), 1, 1]);
expect(child.mock.calls[2]).toEqual([expect.any(Function), 3, 1]);

supportsMock.mockRestore();
});
Expand Down
36 changes: 23 additions & 13 deletions src/utils/batched-updates.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,23 +8,33 @@ import {
import defaults from '../defaults';
import supports from './supported-features';

let isInsideBatchedSchedule = false;
let batchedScheduled = false;

let batched = [];

const executeBatched = () => {
unstable_batchedUpdates(() => {
while (batched.length) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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, 👍

const currentBatched = batched;
batched = [];
currentBatched.forEach((fn) => fn());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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? 🤔

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds logical, batch can handle queue, but there will be a different in behavior

  • now schedule executed all notifications at once
  • then batch will 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

}
// important to reset it before exiting this function
// as React will dump everything right after
batchedScheduled = false;
Copy link
Author

@theKashey theKashey Dec 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • 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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's maybe put this in a minor, but eager to test in Jira and see what breaks there 🙈

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

});
};

export function batch(fn) {
// if we are in node/tests or nested schedule
if (
!defaults.batchUpdates ||
!supports.scheduling() ||
isInsideBatchedSchedule
) {
if (!defaults.batchUpdates || !supports.scheduling()) {
return unstable_batchedUpdates(fn);
}

isInsideBatchedSchedule = true;
// Use ImmediatePriority as it has -1ms timeout
// https://github.com/facebook/react/blob/main/packages/scheduler/src/forks/Scheduler.js#L65
return scheduleCallback(ImmediatePriority, function scheduleBatchedUpdates() {
unstable_batchedUpdates(fn);
isInsideBatchedSchedule = false;
});
batched.push(fn);

if (!batchedScheduled) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

having batchedScheduled being implemented in a wrong way first time (note the comment before resetting it to false) - I am full in 🚀

batchedScheduled = true;
return scheduleCallback(ImmediatePriority, executeBatched);
}
}
Loading