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

State updates outside of act interfere with subsequent state updates during act #1794

Closed
robertknight opened this issue Jul 23, 2019 · 6 comments · Fixed by #1851
Closed

Comments

@robertknight
Copy link
Member

A state update to a component scheduled outside of an act() callback can prevent a state update scheduled inside of an act() callback from executing synchronously as it should:

Steps to reproduce:

import { createElement, render } from "preact";
import { useState, useEffect } from "preact/hooks";
import { act } from "preact/test-utils";

function App({ foo }) {
  const [fooState, setFooState] = useState(null);
  useEffect(() => {
    setFooState(foo);
  }, [foo, setFooState]);

  return <div>{fooState}</div>;
}

const container = document.querySelector("#app");

render(<App foo={1} />, container);
act(() => {
  render(<App foo={2} />, container);
});
console.log("initial render output", container.innerHTML);

Runnable demo: https://codesandbox.io/s/preact-10-state-update-after-unmount-y1fj0

nb. With this sandbox I see different outputs in the DOM when viewing the sandbox output in an iframe vs a new window. In the iframe the DOM output matches the console log. In a new window, the DOM updates after the console log to show "2".

Expected output:

"initial render output <div>2</div>"

Actual output:

"initial render output <div>1</div>"

Notes:

When setState is called or a state update hook runs, the enqueueRender function is called. This function uses the options.debounceRendering hook which is overridden in the context of an act call. However, options.debounceRendering is only called if the deferred rendering queue is empty. In the above example, when the component is updated inside the act call, the queue is non-empty so rendering is deferred.

The above example was extracted from a test where a state update was being triggered in a timeout, outside of an act call, to work around an issue with CSS transitions in certain browsers. This surprisingly interfered with later tests due to the global deferred render queue.

@robertknight robertknight changed the title State updates scheduled outside of act calls can prevent state updates inside of act calls from executing synchronously State updates outside of act interfere with subsequent state updates during act Jul 23, 2019
@JoviDeCroock
Copy link
Member

JoviDeCroock commented Jul 23, 2019

As far as I know it is because your first render should also be incorporated into act since this triggers hooks without an updated options.debounceRendering.

We changed act to be atomic, so no permanent changes to options only during the callback and all related updates inside of that callback.

Works fine when I change it to:

		act(() => {
			render(<App foo={1} />, scratch);
		});
		act(() => {
			render(<App foo={2} />, scratch);
		});

@robertknight
Copy link
Member Author

A conceptually very similar issue exists with useEffect and options.requestAnimationFrame as it uses similar deferred update logic:

if (!component._afterPaintQueued && (component._afterPaintQueued = true) && afterPaintEffects.push(component) === 1) {

@robertknight
Copy link
Member Author

As far as I know it is because your first render should also be incorporated into act since this triggers hooks without an updated options.debounceRendering.

In the context that I extracted this test case from, the state update is triggered asynchronously inside a useEffect handler using setTimeout, so it is difficult to wrap in act. I'm sure I could figure out something for that specific test. The bigger issue though is that this issue creates a footgun which is very confusing to debug because something that one test does can affect what happens in later, unrelated tests, due to the global queues used for deferred rendering.

@robertknight
Copy link
Member Author

One possible solution might be that an act() call flushes any existing deferred renders before it executes. This would require the core Preact package and the hooks package to export functions to flush their respective queues.

@developit
Copy link
Member

@robertknight I wonder if that behavior could be the default for act()? Something like act(true) could throw when state updates were enqueued that would otherwise fall into the next act-triggered sequence.

robertknight added a commit that referenced this issue Aug 10, 2019
When scheduling a state update or effect, check whether the
`options.{debounceRendering, requestAnimationFrame}` hooks changed since
an update/effect queue flush was last scheduled and schedule a flush
using the new hooks if so.

This fixes an issue where a call to `act` would fail to synchronously flush
state updates and effects if a flush was already scheduled but not yet executed
before `act` was called. This happened because when a state update or effect was
queued during the `act` callback, if the global update/effect queues were
non-empty then the temporary hooks installed by `act` to facilitate
synchronously flushing the queue would not be called.

More generally, this fixes an issue where any changes to the scheduling hooks
do not take effect until any already-scheduled flushes have happened.

Fixes #1794
robertknight added a commit that referenced this issue Aug 10, 2019
When scheduling a state update or effect, check whether the
`options.{debounceRendering, requestAnimationFrame}` hooks changed since
an update/effect queue flush was last scheduled and schedule a flush
using the new hooks if so.

This fixes an issue where a call to `act` would fail to synchronously flush
state updates and effects if a flush was already scheduled but not yet executed
before `act` was called. This happened because when a state update or effect was
queued during the `act` callback, if the global update/effect queues were
non-empty then the temporary hooks installed by `act` to facilitate
synchronously flushing the queue would not be called.

More generally, this fixes an issue where any changes to the scheduling hooks
do not take effect until any already-scheduled flushes have happened.

Fixes #1794
@robertknight
Copy link
Member Author

On reflection I realized that there was a more general issue here to do with the way the scheduling hooks (options.{debounceRendering, requestAnimationFrame}) are handled that could break other code that temporarily changes these hooks for any reason.

robertknight added a commit to hypothesis/lms that referenced this issue Sep 10, 2019
This should fix a cause of flakey tests [1]

[1] preactjs/preact#1794
robertknight added a commit to hypothesis/lms that referenced this issue Sep 10, 2019
This should fix a cause of flakey tests [1]

[1] preactjs/preact#1794
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants