[Flight / Flight Reply] Encode Iterator separately from Iterable #28854
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Just giving it a special tag and indirect reference for this edge case.
facebook-github-bot
added
CLA Signed
React Core Team
sebmarkbage
commented
Apr 17, 2024
| @@ -1911,7 +1921,13 @@ function renderModelDestructive( | |||
|
|
|||
| const iteratorFn = getIteratorFn(value); | |||
| if (iteratorFn) { | |||
| return renderFragment(request, task, Array.from((value: any))); | |||
| // TODO: Should we serialize the return value as well like we do for AsyncIterables? | |||
There was a problem hiding this comment.
This is kind of annoying because it would require more code. We could potentially just not support it in the AsyncIterable case too.
|
The size diff is too large to display in a single comment. The CircleCI job contains an artifact called 'sizebot-message.md' with the full message. |
This was referenced Apr 17, 2024
gnoff
approved these changes
Apr 19, 2024
sebmarkbage
added a commit
that referenced
this pull request
Apr 21, 2024
Stacked on #28853 and #28854. React supports rendering `Iterable` and will soon support `AsyncIterable`. As long as it's multi-shot since during an update we may have to rerender with new inputs an loop over the iterable again. Therefore the `Iterator` and `AsyncIterator` types are not supported directly as a child of React - and really it shouldn't pass between Hooks or components neither for this reason. For parity, that's also the case when used in Server Components. However, there is a special case when the component rendered itself is a generator function. While it returns as a child an `Iterator`, the React Element itself can act as an `Iterable` because we can re-evaluate the function to create a new generator whenever we need to. It's also very convenient to use generator functions over constructing an `AsyncIterable`. So this is a proposal to special case the `Generator`/`AsyncGenerator` returned by a (Async) Generator Function. In Flight this means that when we render a Server Component we can serialize this value as an `Iterable`/`AsyncIterable` since that's effectively what rendering it on the server reduces down to. That way if Fiber can receive the result in any position. For SuspenseList this would also need another special case because the children of SuspenseList represent "rows". `<SuspenseList><Component /></SuspenseList>` currently is a single "row" even if the component renders multiple children or is an iterator. This is currently different if Component is a Server Component because it'll reduce down to an array/AsyncIterable and therefore be treated as one row per its child. This is different from `<SuspenseList><Component /><Component /></SuspenseList>` since that has a wrapper array and so this is always two rows. It probably makes sense to special case a single-element child in `SuspenseList` to represent a component that generates rows. That way you can use an `AsyncGeneratorFunction` to do this.
github-actions bot
pushed a commit
that referenced
this pull request
Apr 21, 2024
Stacked on #28853 and #28854. React supports rendering `Iterable` and will soon support `AsyncIterable`. As long as it's multi-shot since during an update we may have to rerender with new inputs an loop over the iterable again. Therefore the `Iterator` and `AsyncIterator` types are not supported directly as a child of React - and really it shouldn't pass between Hooks or components neither for this reason. For parity, that's also the case when used in Server Components. However, there is a special case when the component rendered itself is a generator function. While it returns as a child an `Iterator`, the React Element itself can act as an `Iterable` because we can re-evaluate the function to create a new generator whenever we need to. It's also very convenient to use generator functions over constructing an `AsyncIterable`. So this is a proposal to special case the `Generator`/`AsyncGenerator` returned by a (Async) Generator Function. In Flight this means that when we render a Server Component we can serialize this value as an `Iterable`/`AsyncIterable` since that's effectively what rendering it on the server reduces down to. That way if Fiber can receive the result in any position. For SuspenseList this would also need another special case because the children of SuspenseList represent "rows". `<SuspenseList><Component /></SuspenseList>` currently is a single "row" even if the component renders multiple children or is an iterator. This is currently different if Component is a Server Component because it'll reduce down to an array/AsyncIterable and therefore be treated as one row per its child. This is different from `<SuspenseList><Component /><Component /></SuspenseList>` since that has a wrapper array and so this is always two rows. It probably makes sense to special case a single-element child in `SuspenseList` to represent a component that generates rows. That way you can use an `AsyncGeneratorFunction` to do this. DiffTrain build for [5b903cd](5b903cd)
sebmarkbage
added a commit
that referenced
this pull request
Apr 22, 2024
…ent Components (#28868) Stacked on #28849, #28854, #28853. Behind a flag. If you're following along from the side-lines. This is probably not what you think it is. It's NOT a way to get updates to a component over time. The AsyncIterable works like an Iterable already works in React which is how an Array works. I.e. it's a list of children - not the value of a child over time. It also doesn't actually render one component at a time. The way it works is more like awaiting the entire list to become an array and then it shows up. Before that it suspends the parent. To actually get these to display one at a time, you have to opt-in with `<SuspenseList>` to describe how they should appear. That's really the interesting part and that not implemented yet. Additionally, since these are effectively Async Functions and uncached promises, they're not actually fully "supported" on the client yet for the same reason rendering plain Promises and Async Functions aren't. They warn. It's only really useful when paired with RSC that produces instrumented versions of these. Ideally we'd published instrumented helpers to help with map/filter style operations that yield new instrumented AsyncIterables. The way the implementation works basically just relies on unwrapThenable and otherwise works like a plain Iterator. There is one quirk with these that are different than just promises. We ask for a new iterator each time we rerender. This means that upon retry we kick off another iteration which itself might kick off new requests that block iterating further. To solve this and make it actually efficient enough to use on the client we'd need to stash something like a buffer of the previous iteration and maybe iterator on the iterable so that we can continue where we left off or synchronously iterate if we've seen it before. Similar to our `.value` convention on Promises. In Fizz, I had to do a special case because when we render an iterator child we don't actually rerender the parent again like we do in Fiber. However, it's more efficient to just continue on where we left off by reusing the entries from the thenable state from before in that case.
github-actions bot
pushed a commit
that referenced
this pull request
Apr 22, 2024
…ent Components (#28868) Stacked on #28849, #28854, #28853. Behind a flag. If you're following along from the side-lines. This is probably not what you think it is. It's NOT a way to get updates to a component over time. The AsyncIterable works like an Iterable already works in React which is how an Array works. I.e. it's a list of children - not the value of a child over time. It also doesn't actually render one component at a time. The way it works is more like awaiting the entire list to become an array and then it shows up. Before that it suspends the parent. To actually get these to display one at a time, you have to opt-in with `<SuspenseList>` to describe how they should appear. That's really the interesting part and that not implemented yet. Additionally, since these are effectively Async Functions and uncached promises, they're not actually fully "supported" on the client yet for the same reason rendering plain Promises and Async Functions aren't. They warn. It's only really useful when paired with RSC that produces instrumented versions of these. Ideally we'd published instrumented helpers to help with map/filter style operations that yield new instrumented AsyncIterables. The way the implementation works basically just relies on unwrapThenable and otherwise works like a plain Iterator. There is one quirk with these that are different than just promises. We ask for a new iterator each time we rerender. This means that upon retry we kick off another iteration which itself might kick off new requests that block iterating further. To solve this and make it actually efficient enough to use on the client we'd need to stash something like a buffer of the previous iteration and maybe iterator on the iterable so that we can continue where we left off or synchronously iterate if we've seen it before. Similar to our `.value` convention on Promises. In Fizz, I had to do a special case because when we render an iterator child we don't actually rerender the parent again like we do in Fiber. However, it's more efficient to just continue on where we left off by reusing the entries from the thenable state from before in that case. DiffTrain build for commit 9f2eebd.
github-actions bot
pushed a commit
that referenced
this pull request
Apr 22, 2024
…ent Components (#28868) Stacked on #28849, #28854, #28853. Behind a flag. If you're following along from the side-lines. This is probably not what you think it is. It's NOT a way to get updates to a component over time. The AsyncIterable works like an Iterable already works in React which is how an Array works. I.e. it's a list of children - not the value of a child over time. It also doesn't actually render one component at a time. The way it works is more like awaiting the entire list to become an array and then it shows up. Before that it suspends the parent. To actually get these to display one at a time, you have to opt-in with `<SuspenseList>` to describe how they should appear. That's really the interesting part and that not implemented yet. Additionally, since these are effectively Async Functions and uncached promises, they're not actually fully "supported" on the client yet for the same reason rendering plain Promises and Async Functions aren't. They warn. It's only really useful when paired with RSC that produces instrumented versions of these. Ideally we'd published instrumented helpers to help with map/filter style operations that yield new instrumented AsyncIterables. The way the implementation works basically just relies on unwrapThenable and otherwise works like a plain Iterator. There is one quirk with these that are different than just promises. We ask for a new iterator each time we rerender. This means that upon retry we kick off another iteration which itself might kick off new requests that block iterating further. To solve this and make it actually efficient enough to use on the client we'd need to stash something like a buffer of the previous iteration and maybe iterator on the iterable so that we can continue where we left off or synchronously iterate if we've seen it before. Similar to our `.value` convention on Promises. In Fizz, I had to do a special case because when we render an iterator child we don't actually rerender the parent again like we do in Fiber. However, it's more efficient to just continue on where we left off by reusing the entries from the thenable state from before in that case. DiffTrain build for [9f2eebd](9f2eebd)
This was referenced Apr 24, 2024
bigfootjon
pushed a commit
that referenced
this pull request
Apr 25, 2024
…ent Components (#28868) Stacked on #28849, #28854, #28853. Behind a flag. If you're following along from the side-lines. This is probably not what you think it is. It's NOT a way to get updates to a component over time. The AsyncIterable works like an Iterable already works in React which is how an Array works. I.e. it's a list of children - not the value of a child over time. It also doesn't actually render one component at a time. The way it works is more like awaiting the entire list to become an array and then it shows up. Before that it suspends the parent. To actually get these to display one at a time, you have to opt-in with `<SuspenseList>` to describe how they should appear. That's really the interesting part and that not implemented yet. Additionally, since these are effectively Async Functions and uncached promises, they're not actually fully "supported" on the client yet for the same reason rendering plain Promises and Async Functions aren't. They warn. It's only really useful when paired with RSC that produces instrumented versions of these. Ideally we'd published instrumented helpers to help with map/filter style operations that yield new instrumented AsyncIterables. The way the implementation works basically just relies on unwrapThenable and otherwise works like a plain Iterator. There is one quirk with these that are different than just promises. We ask for a new iterator each time we rerender. This means that upon retry we kick off another iteration which itself might kick off new requests that block iterating further. To solve this and make it actually efficient enough to use on the client we'd need to stash something like a buffer of the previous iteration and maybe iterator on the iterable so that we can continue where we left off or synchronously iterate if we've seen it before. Similar to our `.value` convention on Promises. In Fizz, I had to do a special case because when we render an iterator child we don't actually rerender the parent again like we do in Fiber. However, it's more efficient to just continue on where we left off by reusing the entries from the thenable state from before in that case. DiffTrain build for commit 9f2eebd.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
For
AsyncIterablewe encodeAsyncIteratoras a separate tag.Previously we encoded
Iteratoras just an Array. This adds a special encoding for this. Technically this is a breaking change.This is kind of an edge case that you'd care about the difference but it becomes more important to treat these correctly for the warnings here #28853.