Bug: useTransition's pending boolean is triggered immediately, even if there's no suspension

[arackaf]

React version: 241c446

Steps To Reproduce

1. https://codesandbox.io/s/cranky-wing-lc7wr
2. Swap between the two screens by clicking the two buttons
3. Inline loading shows the first time you load screen A. But it also usually shows on subsequent loads, even though there's nothing Suspending

Link to code example:

The current behavior

Inline loading indicator usually shows on subsequent loads.

The expected behavior

It should only show on the first load of A

[gaearon]

First guess: we're running into a CPU timeout there and flash the spinner. I wonder if we should adjust some heuristic so it doesn't happen that often though.

[gaearon]

My understanding is that useTransition does not only track if something is suspended, but also just if something took too long to render. For example I'm not seeing the inline spinner if we only have ~500 items in the list. Keep in mind that in production mode the rendering itself would also happen faster so that the limit would be bigger.

I think it's fair to say that once we do show an inline spinner, it's bad to update the screen too fast after that. Even if we're ready. There's an undocumented busyMinDurationMs option that does this. Like in https://codesandbox.io/s/magical-cloud-1il9r, I set it to 100, and now the flash is not as jarring. The compromise is that now it delays you for a bit longer from seeing the content, and you see it regardless of how much work it took to render.

I'll keep this open because there's probably more we could do there. E.g. keep the pending state around for longer by default, but prevent it from triggering if we didn't wait enough. There's probably better heuristics we could use.

[arackaf]

@gaearon looking at this again with fresh eyes, I definitely think there's a heuristic tweak needed. I've got a live demo up and running here: https://mylibrary.io/view?userId=573d1b97120426ef0078aa92

If you page up a few pages, and then page back down, you'll see cached results loading back up. On my MacBook on Chrome, the cached pages load as close to instantly as is probably possible, but I still get a flash of that spinner.

Here's an even better example: https://mylibrary.io/view?page=2&subjects=583087fa8d9110f200d29f5d&userId=573d1b97120426ef0078aa92

Should be two total pages, page 2 should have 6 entries on it. Loading page 2 (cached) sometimes has the spinner not show, but it's always (again for me, essentially) instantaneous.

[gaearon]

Took another look at this — yea, I'm pretty sure this is because we're committing the initial isPending = true state right away, and so it might flash. The workaround that I've used so far for this is just to delay the spinner with CSS and then use CSS transition to make it appear gradually. So even if it flashes briefly, it's barely noticeable.

https://codesandbox.io/s/busy-smoke-rjnrk?file=/src/index.js:358-563

const Spinner = ({ visible }) => (
  <div
    style={{
      transition: visible ? "opacity 0.2s 0.2s linear" : "opacity 0s 0s linear",
      opacity: visible ? 1 : 0
    }}
  >
    loading...
  </div>
);

This is kind of a hack though. Let's keep this open. It's on my list of things to ask the team about as we get closer to a Beta.

Nope, that's the solution (if you want to keep it a spinner): #18006 (comment).

[arackaf]

This is kind of a hack though. Let's keep this open. It's on my list of things to ask the team about as we get closer to a Beta.

Thanks @gaearon I appreciate you looking, and keeping the issue open. I'm relieved to hear you say it's a hack, which of course is more than good enough for something that's still pre-beta. But care-free spinner state was always one of the most loudly touted features of Suspense, so I really hope this gets buttoned up before it ships.

Take care!

[bvaughn]

@arackaf Your original Sandbox link is broken for me, but I copied the code to a new one and pointed it to the 18 beta release:
https://codesandbox.io/s/vigilant-lichterman-6uwks?file=/src/App.js

Does it look good to you now?

[gaearon]

No, this still reproducible if you switch to createRoot: https://codesandbox.io/s/agitated-wood-s202b?file=/src/App.js

My understanding is that it works as (currently) designed and is not a bug. Transitions don't only wait for suspending; they also unblock regular rendering. So the first commit always has isPending = true so that we can provide feedback immediately — even if the transition itself doesn't take long. This is because we can't guess how long it's going to take. It could suspend, or it could have heavy rendering, or both.

I wonder what behavior would be preferred instead — not in this single case but in general. Let's assume the (existing) assumption that isPending corresponds to both CPU (rendering) and IO (network) delays. Would we expect the initial isPending = true to be delayed? But even if we e.g. delay isPending = true by 100ms, this doesn't solve the problem. Sure, it would avoid a flash if the work takes 50ms. But if the work takes 120ms, then we still have the same annoying 20ms flash. So delaying isPending does not solve the problem. Another approach could be to delay resetting it back to false. This is something we had an option for but we removed it for now. Note that if we just delay its resetting back to false, we'll have to show an inconsistent state where you've already transitioned but isPending = true. That seems confusing. Maybe we could "hold back" the commit with the actual transition. This might be what we'll do when we have first-class exit animation support. But note that each transition would be penalized because none of them would switch synchronously.

I suspect that the thing you're really looking for is more granular control and make it so that isPending only becomes true if something suspends (regardless of whether CPU rendering takes a while). I'm not sure about this direction. We like that our programming model lets us think about CPU and IO work in a unified way, and having one API for them is powerful. Note that even if there was more granular control, this problem would still exist in the async case where resolving async work is fast — you'd have a similar flash even then. So maybe this helps in this particular case but it doesn't seem like a general solution. Similar, this wouldn't solve the same problem for when you do want to unblock rendering (CPU work) and it happens to be fast enough, causing a flash. This seems unsatisfactory.

So far, my impression is that the CSS solution in #18006 (comment) is most flexible and I would recommend that for now. This issue is still on our list of things to look at. So we'll likely revisit this when/if we have better ideas around it.

[bvaughn]

No, this still reproducible if you switch to createRoot: https://codesandbox.io/s/agitated-wood-s202b?file=/src/App.js

Damn. Silly oversight.

I wonder what behavior would be preferred instead — not in this single case but in general. Let's assume the (existing) assumption that isPending corresponds to both CPU (rendering) and IO (network) delays. Would we expect the initial isPending = true to be delayed? But even if we e.g. delay isPending = true by 100ms, this doesn't solve the problem. Sure, it would avoid a flash if the work takes 50ms. But if the work takes 120ms, then we still have the same annoying 20ms flash.

This matches my thinking (and I think what I said to Adam when we talked about this in person the other week).

[gaearon]

I've chatted a bit more to Sebastian about it today. There's a few things here.

Our early communication over-emphasized spinners and that kind of became the default way of thinking about it. But in many designs they don't quite make sense.

For transitions, in many cases it's reasonable to expect that isPending would not last very long. So when you use it, you might want to use it more in a way similar to a button's "active" / "pressed" state — a subtle hint that something's happening, but not necessarily showing a whole new element like a spinner. I'm not a designer but here's a mockup with making the button appear "pressed" while the transition is pending: https://codesandbox.io/s/ancient-meadow-ndptu?file=/src/App.js. For transition feedback like this, it isn't jarring that it holds for a little amount of time. It's kind of a "post-pressed" state.

For designs with spinners, the solution with CSS #18006 (comment) is the canonical solution. The great thing about it is that it works regardless of how long the work actually takes. If it takes too little, you simply won't notice the barely visible spinner. If it takes long enough, then you've had a chance to see it fully. You can also delay the transition itself with CSS. This lets you completely "skip" showing it for fast transitions. Additionally, since the CSS transition can be accelerated, you're not stealing time from CPU from the important work of actually finishing the render, as you would be with some JS-based solution.

So we should be able to close this. There's definitely a need to document this pattern (and similar "how to think about X") though. But I think we have a conclusion on this issue.

[arackaf]

@bvaughn @gaearon thanks for looking at this (again). Sorry I wasn’t more responsive - was traveling when you guys took this up :-)