Warn for Child Iterator of all types but allow Generator Components #28853
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
facebook-github-bot
added
CLA Signed
React Core Team
|
Comparing: 7909d8e...e30e21a Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: Expand to show
|
sebmarkbage
force-pushed
the
iteratorchildren
branch
from
38621bc to
e30e21a
Compare
gnoff
approved these changes
Apr 17, 2024
Comment on lines
+1253
to
+1255
| knownKeys = __DEV__ | ||
| ? warnOnInvalidKey(step.value, knownKeys, returnFiber) | ||
| : null |
There was a problem hiding this comment.
Might DCE better if you move this to the first line of the for block. It's not big but it leaves behind a null in the prod builds for each instance
There was a problem hiding this comment.
The way this is structured, that doesn't mean the same thing because this should only execute at the beginning of the next iteration loop - not the first one. A proper structure would need to change the way the continue calls are placed.
acdlite
approved these changes
Apr 17, 2024
This was referenced Apr 17, 2024
sebmarkbage
added a commit
that referenced
this pull request
Apr 21, 2024
) For [`AsyncIterable`](#28847) we encode `AsyncIterator` as a separate tag. Previously we encoded `Iterator` as 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.
github-actions bot
pushed a commit
that referenced
this pull request
Apr 21, 2024
…28853) This doesn't change production behavior. We always render Iterables to our best effort in prod even if they're Iterators. But this does change the DEV warnings which indicates which are valid patterns to use. It's a footgun to use an Iterator as a prop when you pass between components because if an intermediate component rerenders without its parent, React won't be able to iterate it again to reconcile and any mappers won't be able to re-apply. This is actually typically not a problem when passed only to React host components but as a pattern it's a problem for composability. We used to warn only for Generators - i.e. Iterators returned from Generator functions. This adds a warning for Iterators created by other means too (e.g. Flight or the native Iterator utils). The heuristic is to check whether the Iterator is the same as the Iterable because that means it's not possible to get new iterators out of it. This case used to just yield non-sense like empty sets in DEV but not in prod. However, a new realization is that when the Component itself is a Generator Function, it's not actually a problem. That's because the React Element itself works as an Iterable since we can ask for new generators by calling the function again. So this adds a special case to allow the Generator returned from a Generator Function's direct child. The principle is “don’t pass iterators around” but in this case there is no iterator floating around because it’s between React and the JS VM. Also see #28849 for context on AsyncIterables. Related to this, but Hooks should ideally be banned in these for the same reason they're banned in Async Functions. DiffTrain build for commit 3682021.
github-actions bot
pushed a commit
that referenced
this pull request
Apr 21, 2024
…28853) This doesn't change production behavior. We always render Iterables to our best effort in prod even if they're Iterators. But this does change the DEV warnings which indicates which are valid patterns to use. It's a footgun to use an Iterator as a prop when you pass between components because if an intermediate component rerenders without its parent, React won't be able to iterate it again to reconcile and any mappers won't be able to re-apply. This is actually typically not a problem when passed only to React host components but as a pattern it's a problem for composability. We used to warn only for Generators - i.e. Iterators returned from Generator functions. This adds a warning for Iterators created by other means too (e.g. Flight or the native Iterator utils). The heuristic is to check whether the Iterator is the same as the Iterable because that means it's not possible to get new iterators out of it. This case used to just yield non-sense like empty sets in DEV but not in prod. However, a new realization is that when the Component itself is a Generator Function, it's not actually a problem. That's because the React Element itself works as an Iterable since we can ask for new generators by calling the function again. So this adds a special case to allow the Generator returned from a Generator Function's direct child. The principle is “don’t pass iterators around” but in this case there is no iterator floating around because it’s between React and the JS VM. Also see #28849 for context on AsyncIterables. Related to this, but Hooks should ideally be banned in these for the same reason they're banned in Async Functions. DiffTrain build for [3682021](3682021)
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
…28853) This doesn't change production behavior. We always render Iterables to our best effort in prod even if they're Iterators. But this does change the DEV warnings which indicates which are valid patterns to use. It's a footgun to use an Iterator as a prop when you pass between components because if an intermediate component rerenders without its parent, React won't be able to iterate it again to reconcile and any mappers won't be able to re-apply. This is actually typically not a problem when passed only to React host components but as a pattern it's a problem for composability. We used to warn only for Generators - i.e. Iterators returned from Generator functions. This adds a warning for Iterators created by other means too (e.g. Flight or the native Iterator utils). The heuristic is to check whether the Iterator is the same as the Iterable because that means it's not possible to get new iterators out of it. This case used to just yield non-sense like empty sets in DEV but not in prod. However, a new realization is that when the Component itself is a Generator Function, it's not actually a problem. That's because the React Element itself works as an Iterable since we can ask for new generators by calling the function again. So this adds a special case to allow the Generator returned from a Generator Function's direct child. The principle is “don’t pass iterators around” but in this case there is no iterator floating around because it’s between React and the JS VM. Also see #28849 for context on AsyncIterables. Related to this, but Hooks should ideally be banned in these for the same reason they're banned in Async Functions. DiffTrain build for commit 3682021.
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.
This doesn't change production behavior. We always render Iterables to our best effort in prod even if they're Iterators.
But this does change the DEV warnings which indicates which are valid patterns to use.
It's a footgun to use an Iterator as a prop when you pass between components because if an intermediate component rerenders without its parent, React won't be able to iterate it again to reconcile and any mappers won't be able to re-apply. This is actually typically not a problem when passed only to React host components but as a pattern it's a problem for composability.
We used to warn only for Generators - i.e. Iterators returned from Generator functions. This adds a warning for Iterators created by other means too (e.g. Flight or the native Iterator utils). The heuristic is to check whether the Iterator is the same as the Iterable because that means it's not possible to get new iterators out of it. This case used to just yield non-sense like empty sets in DEV but not in prod.
However, a new realization is that when the Component itself is a Generator Function, it's not actually a problem. That's because the React Element itself works as an Iterable since we can ask for new generators by calling the function again. So this adds a special case to allow the Generator returned from a Generator Function's direct child. The principle is “don’t pass iterators around” but in this case there is no iterator floating around because it’s between React and the JS VM.
Also see #28849 for context on AsyncIterables.
Related to this, but Hooks should ideally be banned in these for the same reason they're banned in Async Functions.