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.

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(batching): improve batching behavior to work automatically in all cases. #226

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

Conversation

k1w1
Copy link

@k1w1 k1w1 commented Jan 20, 2021

Resolves #186

Proposed changes:

  • automatically include all component renders within a batch
  • removes all monkey patching to do batching of component renders

The basic theory of operation is captured in view.js. Whenever a component re-render is needed record that fact on a per-component-instance basis. When the current task finishes executing then perform all of the re-renders at once. It uses a microTask to detect the end of the current task (rather than using setTimeout) so that the re-renders happen before any other event handlers - so that render is not unduly delayed if the event queue is full, e.g. during a drag operation.

Overall this means that changes are appropriately batched together regardless of the execution context. I think that in practice it also means that the batch method is probably not necessary, but I did not remove it or change its behavior since there may be some subtle aspects to existing code that depend on it.

I had to add some helpers to the tests, and modify all of the tests, to handle microTasks and to run the timers before checking the test output.

The tests pass. I have also tested this against our real-world use of react-easy-state without finding any problems.

k1w1 added 5 commits January 17, 2021 08:36
…riggered. Run the batch as a microtask at the end of the current task.
I hit a problem with the edge cases and out-of-order execution of updates. I need to use React batching of changes, which requires using only one micro task for all views, instead of a separate micro task for each view as it is now.
When react-easy-state changes are triggered from the main event loop,
they must run inside the main event loop, instead of being batched in
the microtask. Otherwise, the cursor will jump to the end of input
elements, since the change can't be tied back to the action
taken. React batches changes caused inside an event loop, so we can
rely on the normal React setState batching.
@justinweiss justinweiss force-pushed the cjw-batching-all-changes branch from 286f77a to 31e541c Compare April 5, 2022 19:17
React automatically batches setState calls, but only inside its own
event handlers. If we know we've scheduled a batch update, we should
run the changes using batchedUpdates -- at the end of an event loop,
if we're in an event, or in the microtask if we're not.
The previous fix still had the input element problem in React
production builds. This way we guarantee updates will run in the way
React expects them to.
…er changes"

This reverts commit 6203429.

That change caused incorrect cursor movement in many more places, and
is not a correct fix to the original problem.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Render re-entering after async function
2 participants