Add tail="collapsed" option to SuspenseList #16007
Conversation
|
ReactDOM: size: 0.0%, gzip: -0.1% Details of bundled changes.Comparing: 933c664...ccdb8b0 react-dom
react-art
react-test-renderer
react-native-renderer
react-reconciler
Generated by 🚫 dangerJS |
sebmarkbage
force-pushed
the
suspenselist
branch
from
8ffa724
to
0bbef6b
Compare
This API was my initial intuition as well. I'm not sure I understand the statement about requiring two fibers. Wouldn't nested lists also require two fibers? |
There was a problem hiding this comment.
I only reviewed the delta between the two branches and I don't have as much context on this feature anyway, so take my feedback with a grain of salt.
| 'Did you mean "collapsed"?', | ||
| tailOptions, | ||
| ); | ||
| } else if (revealOrder !== 'forwards' && revealOrder !== 'backwards') { |
There was a problem hiding this comment.
So that you don't get both warnings at once. Seems more actionable to pick a legit value first. E.g. if you specified something like "none", it would be misleading to recommend a revealOrder.
There was a problem hiding this comment.
Ah. I guess that makes sense. I was worried it could feel a bit like warning-whack-a-mole if you fixed one just to see another pop up.
The revealOrder would still be invalid if you specified tail="none" though so wouldn't we still want to warn about it?
I dunno. I don't feel strongly here 😄 Was just asking.
| break; | ||
| } | ||
| default: { | ||
| // TODO: warn if not undefined |
There was a problem hiding this comment.
You already warned in begin.
| @@ -24,6 +26,8 @@ export type SuspenseListState = {| | |||
| tail: null | Fiber, | |||
| // The absolute time in ms that we'll expire the tail rendering. | |||
| tailExpiration: number, | |||
| // Tail insertions setting. | |||
| tailOptions: SuspenseListTailOptions, | |||
There was a problem hiding this comment.
Trivial naming nit: I think of "options" as being a config object. This strikes me as more of a "mode" or a "type"
| jest.advanceTimersByTime(500); | ||
|
|
||
| // Even though B is unsuspended, it's still in loading state because | ||
| // it is blocked by C. |
There was a problem hiding this comment.
Seems counter-intuitive to me that B is blocked by C in this case. Since we display both loading states, I would also have expected them to resolve in order.
There was a problem hiding this comment.
This isn't new behavior in this PR and this is basically just replicating the same assertion as other tests. The rationale for this behavior is that in a variable size list you don't want to pop in things multiple time to shift the below content around multiple times. Therefore the "head" resolves in the "together" mode.
This is kind of a rare case though since insertions in the middle are rare. Let alone multiple of them. A more common variant is inserting at the top but in that case you don't want to start with the top most and then expand them one by one to shift things down. More likely it's better to just show them all as a unit.
| <span>C</span> | ||
| </Fragment>, | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Worth adding some tests that resolve things out of order?
There was a problem hiding this comment.
Some of the other ones resolve out of order, but I don't really want to add every combination. That's more for a fuzz tester.
Implementation detail wise there's also not much to test because we don't even render the future items so we have not attached any listeners.
There was a problem hiding this comment.
Okay. Implementation details can always change though 😁
We used to assume that this didn't suspend but this branch happens in both cases. This fixes it so that we first check if we suspended. Now we can fix the tail so that it always render an additional fallback in this scenario.
|
Found and fixed one more bug. ccdb8b0 |
* Add tail="collapsed" option * Fix issue with tail exceeding the CPU time limit We used to assume that this didn't suspend but this branch happens in both cases. This fixes it so that we first check if we suspended. Now we can fix the tail so that it always render an additional fallback in this scenario.
Builds on top of #16005.
This add an option to SuspenseList which ensures that we avoid inserting any new rows at the tail of the list past some point.
The first option is "collapsed" which means that there is only one row visible at the end of the list. I plan on adding a "hidden" option which ensures that zero visible rows are at the end.
Note that the tail in terms of unfolding row-by-row is defined as any insertions at the bottom or any rows that updated to become suspended or were already inserted from previous commits. This option doesn't actually remove or hide those. However, this is a rare edge cases. Typically you're expected to clear the list for these cases.
The use case here is for streaming rendering of items. This lets you provide more rows to React than you have available yet. The main purpose of this is for server rendering where you can't update to add more. However, it also is useful so that React can prerender later rows while blocked on previous rows. This PR doesn't actually do that yet tho.
This option also allows to render CPU bound work, one item at a time without showing all the fallbacks. Thanks to the tail expiration time.
Collapsing to the last row
The
tail="collapsed"option uses the first new row for showing the fallback state. That's fairly efficient because we've rendered it when we tried the last row.If you want to instead show the last row, you have to use a nested suspense list and the "hidden" option of the inner one:
Tail in this case can be a Suspense boundary that unsuspends when we're not expected to get more items added to the list.
This technique ensures that if
endOfTheListresolves on the server, the SSR streaming can hide the tail shimmer. No need for client rerenders. Placing it in an outer SuspenseList also ensures that onceendOfTheListis resolved we don't hide the Shimmer before the rows in the list are fully loaded. Otherwise, the shimmer would hide first and then we'd wait for the inner rows but they're hidden.This is a bit unfortunate since I think this is a pretty common use case for visualizing a tail load differently from suspended existing boundaries.
In theory we could add an option to collapse into the last row but it gets tricky. Because after the first commit, that row would now be mounted and now it's not a new insertion anymore. That complicates the semantics a bit. The collapsed tail would have to be something like everything after the last committed row that isn't the last one.
This approach doesn’t actually work:
Because that’s actually just two rows since the fragment in that case is treated as a single row.
I feel like it's just clearer with two lists. That also makes it clearer what a "row" means. E.g. if your tail has multiple rows, it kind of just works.
I also suspect that at some point SuspenseList will accept a custom streaming data type and in that case it might be easier to think of the inner list as one stream and the outer as another.
Although a counter point is that virtualization might make this api difficult/impossible to use.
A possible option would be to add an explicit tail component:
However, that would require two fibers to be stored similar to how Suspense inserts extra fibers.