React: Bug: startTransition suspends immediately when useLayoutEffect is present

[arackaf]

React version: 0.0.0-experimental-f42431abe

Please note that I do realize my repro steps are poor at best. I'm not filing this issue in hopes of support; I'm only filing this issue to provide one more datum point to help diagnose what I believe to be a bug, which I'm assuming you'll see more reports of.

Steps To Reproduce

tl;dr - there are some circumstances when a thrown promise inside a hook causes an immediate suspense, instead of respecting the startTransition it's inside of.

startTransition for me is always, in this case, called outside of the normal React handlers. In this case history.listen

https://github.com/arackaf/booklist/blob/special/suspense-blog/react/modules/books/booksSearchState.ts#L75

This is my Suspense-enabled hook that's called as a result of the state update inside the code above

https://github.com/arackaf/micro-graphql-react/blob/feature/suspense/src/useQuery.js#L22

the Promise throwing happens here

https://github.com/arackaf/micro-graphql-react/blob/feature/suspense/src/queryManager.js#L116

Here's the specific chain of events that leads to the breakage.

Things work so long as there's always been an existing promise, for the queryManager to throw, ie line 116 in the code immediately above. But the minute there's cached results, and the queryManager invoke's the hook's setState method (which it passed it), ie line 120 here

https://github.com/arackaf/micro-graphql-react/blob/feature/suspense/src/queryManager.js#L120

then all future suspenses immediately start suspending, and I always get my fallback, always.

The setState method is shared between the hook, and queryManager here

https://github.com/arackaf/micro-graphql-react/blob/feature/suspense/src/useQuery.js#L17

I stress that I do not need support, here; this is just a side project, an unimportant one, and this whole branch is only for Suspense experimenting. But something does definitely appear to be wrong, and I'm hoping this report can help you guys find it.

The current behavior

startTransition only works until the hook's setState is called from the queryManager it creates, at which point startTransition always triggers hard suspenses, showing my fallback.

The expected behavior

startTransition should never trigger the fallback until the timeout has expired.

[arackaf]

Just FYI same behavior on the 241c4467e build

[gaearon]

Can you create a reduced one-file example with no libraries?

[arackaf]

I can try to find some more time to put into this but no promises.

I have to believe there’s something going wrong in how my useQuery hook is sharing the updateState function with the queryManager. I’m most interested in hearing if that’s violating any React constraints which might be causing this.

[arackaf]

@gaearon - I managed to get this reproduced! :D

https://codesandbox.io/s/priceless-shirley-o6sfg

If you click pretty much any of the buttons, you'll often see things suspend.

Note the call to useLayoutEffect on line 83. That's the cause of this. Comment that out, and everything is perfect.

Sorry for all the extra crap in the sandbox - I chased down a ton of false leads here, and honestly don't have the time to clean it all up—I need to get back to work! :)

But I'm stoked to finally be able to give you something hopefully actionable.

Lastly, I do realize the layoutEffect isn't even needed, in this particular case, or probably in my original code which surfaced this odd behavior. But it does seem like a bug, unless I'm missing something.

[arackaf]

Also - h/t to Daishi Kato for helping to get this sandbox started :D

https://twitter.com/dai_shi/status/1223746342289231877

[dai-shi]

So, the layout effect runs when a child component throws a promise. Is it a bug or a behavior.

(Note, my first comment in the twitter thread was pointless.)

[arackaf]

I cleaned up the sandbox for you guys. Offending useLayoutEffect is line 58 now.

I also made the Suspended loader component super obnoxious so it'd be obvious when it only flashes for a moment.

https://codesandbox.io/s/damp-glade-zn8d4

[jddxf]

After digging into this problem for a while, I have found updates scheduled in layout effects would compute a new currentEventTime. That is done by ensureRootIsScheduled before exiting a commit. And currentEventTime wouldn't be reset if there's no remaining work following the updates in layout effects. Then when another batch starts, the retained currentEventTime is used to compute a new expiration time. The more time has elapsed since the last batch, the more likely updates in this batch are going to expire.

Here's a failing test:

const {useState, useLayoutEffect} = React;

let _setCount;
function App() {
  const [count, setCount] = useState(0);
  _setCount = setCount;
  Scheduler.unstable_yieldValue('Render: ' + count);
  useLayoutEffect(() => {
    if (count % 2 === 0) {
      setCount(count + 1);
    }
    Scheduler.unstable_yieldValue('Commit: ' + count);
  }, [count]);
  return null;
}

ReactNoop.act(() => {
  ReactNoop.render(<App />);
  expect(Scheduler).toFlushExpired([]);
  expect(Scheduler).toFlushAndYield(['Render: 0', 'Commit: 0', 'Render: 1', 'Commit: 1']);

  Scheduler.unstable_advanceTime(10000);

  _setCount(3);
  // Should not expire due to layout updates in the last batch
  expect(Scheduler).toFlushExpired([]);
  expect(Scheduler).toFlushAndYield(['Render: 3', 'Commit: 3']);
});

[jeremyscatigna]

Could I help on this one ? 🙂

[gaearon]

@jddxf If you have ideas for a fix, feel free to send one.

[gaearon]

@jddxf Do you mind submitting a failing test as a PR for this?

[jddxf]

@gaearon Opened #18608

[gaearon]

This appears fixed on master.
https://codesandbox.io/s/hardcore-hermann-lnxxi