Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Warn for Child Iterator of all types but allow Generator Components #28853

Merged
merged 1 commit into from
Apr 21, 2024

Conversation

sebmarkbage
Copy link
Collaborator

@sebmarkbage sebmarkbage commented Apr 17, 2024

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.

@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Apr 17, 2024
@react-sizebot
Copy link

react-sizebot commented Apr 17, 2024

Comparing: 7909d8e...e30e21a

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js = 168.91 kB 168.93 kB = 52.94 kB 52.94 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 170.57 kB 170.58 kB = 53.44 kB 53.44 kB
facebook-www/ReactDOM-prod.classic.js = 590.83 kB 590.85 kB = 103.91 kB 103.91 kB
facebook-www/ReactDOM-prod.modern.js = 566.65 kB 566.66 kB = 100.10 kB 100.10 kB
test_utils/ReactAllWarnings.js Deleted 64.44 kB 0.00 kB Deleted 16.10 kB 0.00 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show
Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable-semver/react-server/cjs/react-server.development.js +0.30% 197.19 kB 197.79 kB +0.22% 46.60 kB 46.71 kB
oss-stable/react-server/cjs/react-server.development.js +0.30% 197.19 kB 197.79 kB +0.22% 46.60 kB 46.71 kB
oss-experimental/react-server/cjs/react-server.development.js +0.29% 208.61 kB 209.21 kB +0.22% 48.56 kB 48.66 kB
test_utils/ReactAllWarnings.js Deleted 64.44 kB 0.00 kB Deleted 16.10 kB 0.00 kB

Generated by 🚫 dangerJS against e30e21a

Comment on lines +1253 to +1255
knownKeys = __DEV__
? warnOnInvalidKey(step.value, knownKeys, returnFiber)
: null
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@sebmarkbage sebmarkbage merged commit 3682021 into facebook:main Apr 21, 2024
38 checks passed
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)
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
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants