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

Throw a better error when Lazy/Promise is used in React.Children #28280

Merged
merged 1 commit into from
Feb 8, 2024

Conversation

sebmarkbage
Copy link
Collaborator

@sebmarkbage sebmarkbage commented Feb 8, 2024

We could in theory actually support this case by throwing a Promise when it's used inside a render. Allowing it to be synchronously unwrapped. However, it's a bit sketchy because we officially only support this in the render's child position or in use().

Another alternative could be to actually pass the Promise/Lazy to the callback so that you can reason about it and just return it again or even unwrapping with use() - at least for the forEach case maybe.

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

react-sizebot commented Feb 8, 2024

Comparing: d3def47...56ade53

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 = 176.64 kB 176.64 kB = 55.02 kB 55.02 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 178.63 kB 178.63 kB = 55.59 kB 55.59 kB
facebook-www/ReactDOM-prod.classic.js = 591.73 kB 591.73 kB = 104.44 kB 104.44 kB
facebook-www/ReactDOM-prod.modern.js = 575.51 kB 575.51 kB = 101.53 kB 101.53 kB
oss-stable-semver/react/cjs/react.production.min.js +4.70% 8.06 kB 8.44 kB +3.16% 3.11 kB 3.20 kB
oss-stable/react/cjs/react.production.min.js +4.69% 8.09 kB 8.47 kB +3.10% 3.13 kB 3.23 kB
oss-experimental/react/cjs/react.production.min.js +4.20% 9.02 kB 9.40 kB +2.72% 3.42 kB 3.51 kB
oss-stable-semver/react/umd/react.profiling.min.js +3.21% 11.86 kB 12.24 kB +2.27% 4.63 kB 4.74 kB
oss-stable-semver/react/umd/react.production.min.js +3.20% 11.86 kB 12.24 kB +2.27% 4.63 kB 4.74 kB
oss-stable/react/umd/react.profiling.min.js +3.20% 11.88 kB 12.26 kB +2.28% 4.65 kB 4.76 kB
oss-stable/react/umd/react.production.min.js +3.20% 11.88 kB 12.26 kB +2.28% 4.65 kB 4.76 kB
oss-experimental/react/umd/react.profiling.min.js +2.98% 12.75 kB 13.13 kB +1.74% 4.94 kB 5.02 kB
oss-experimental/react/umd/react.production.min.js +2.98% 12.75 kB 13.13 kB +1.74% 4.94 kB 5.02 kB
facebook-react-native/react/cjs/React-prod.js +2.64% 17.68 kB 18.15 kB +2.14% 4.63 kB 4.73 kB
facebook-react-native/react/cjs/React-profiling.js +2.61% 17.82 kB 18.29 kB +2.12% 4.62 kB 4.72 kB
facebook-www/React-prod.modern.js +2.54% 18.36 kB 18.83 kB +2.06% 4.71 kB 4.81 kB
facebook-www/React-prod.classic.js +2.50% 18.66 kB 19.12 kB +2.02% 4.80 kB 4.90 kB
facebook-www/React-profiling.modern.js +2.48% 18.80 kB 19.26 kB +2.00% 4.80 kB 4.89 kB
facebook-www/React-profiling.classic.js +2.44% 19.09 kB 19.56 kB +2.01% 4.88 kB 4.98 kB
test_utils/ReactAllWarnings.js Deleted 67.02 kB 0.00 kB Deleted 16.42 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/cjs/react.production.min.js +4.70% 8.06 kB 8.44 kB +3.16% 3.11 kB 3.20 kB
oss-stable/react/cjs/react.production.min.js +4.69% 8.09 kB 8.47 kB +3.10% 3.13 kB 3.23 kB
oss-experimental/react/cjs/react.production.min.js +4.20% 9.02 kB 9.40 kB +2.72% 3.42 kB 3.51 kB
oss-stable-semver/react/umd/react.profiling.min.js +3.21% 11.86 kB 12.24 kB +2.27% 4.63 kB 4.74 kB
oss-stable-semver/react/umd/react.production.min.js +3.20% 11.86 kB 12.24 kB +2.27% 4.63 kB 4.74 kB
oss-stable/react/umd/react.profiling.min.js +3.20% 11.88 kB 12.26 kB +2.28% 4.65 kB 4.76 kB
oss-stable/react/umd/react.production.min.js +3.20% 11.88 kB 12.26 kB +2.28% 4.65 kB 4.76 kB
oss-experimental/react/umd/react.profiling.min.js +2.98% 12.75 kB 13.13 kB +1.74% 4.94 kB 5.02 kB
oss-experimental/react/umd/react.production.min.js +2.98% 12.75 kB 13.13 kB +1.74% 4.94 kB 5.02 kB
facebook-react-native/react/cjs/React-prod.js +2.64% 17.68 kB 18.15 kB +2.14% 4.63 kB 4.73 kB
facebook-react-native/react/cjs/React-profiling.js +2.61% 17.82 kB 18.29 kB +2.12% 4.62 kB 4.72 kB
facebook-www/React-prod.modern.js +2.54% 18.36 kB 18.83 kB +2.06% 4.71 kB 4.81 kB
facebook-www/React-prod.classic.js +2.50% 18.66 kB 19.12 kB +2.02% 4.80 kB 4.90 kB
facebook-www/React-profiling.modern.js +2.48% 18.80 kB 19.26 kB +2.00% 4.80 kB 4.89 kB
facebook-www/React-profiling.classic.js +2.44% 19.09 kB 19.56 kB +2.01% 4.88 kB 4.98 kB
oss-stable-semver/react/cjs/react.production.js +1.39% 34.85 kB 35.34 kB +0.95% 9.67 kB 9.77 kB
oss-stable/react/cjs/react.production.js +1.38% 34.88 kB 35.36 kB +0.96% 9.70 kB 9.80 kB
oss-experimental/react/cjs/react.production.js +1.29% 37.49 kB 37.97 kB +0.88% 10.35 kB 10.44 kB
oss-stable-semver/react/cjs/react.react-server.production.min.js +1.23% 6.99 kB 7.07 kB +0.71% 2.98 kB 3.00 kB
oss-stable/react/cjs/react.react-server.production.min.js +1.23% 7.01 kB 7.10 kB +0.73% 3.00 kB 3.02 kB
facebook-www/ReactServer-prod.modern.js +1.14% 14.90 kB 15.07 kB +0.63% 3.99 kB 4.01 kB
oss-experimental/react/cjs/react.react-server.production.min.js +0.95% 9.04 kB 9.12 kB +0.63% 3.68 kB 3.70 kB
oss-stable-semver/react/cjs/react.react-server.production.js +0.72% 29.99 kB 30.21 kB +0.35% 8.94 kB 8.97 kB
oss-stable/react/cjs/react.react-server.production.js +0.72% 30.02 kB 30.23 kB +0.35% 8.96 kB 9.00 kB
oss-stable-semver/react/cjs/react.react-server.development.js +0.62% 77.60 kB 78.08 kB +0.42% 21.43 kB 21.52 kB
oss-stable/react/cjs/react.react-server.development.js +0.62% 77.62 kB 78.10 kB +0.42% 21.46 kB 21.55 kB
oss-experimental/react/cjs/react.react-server.production.js +0.60% 36.11 kB 36.32 kB +0.29% 10.61 kB 10.64 kB
oss-experimental/react/cjs/react.react-server.development.js +0.57% 84.05 kB 84.53 kB +0.39% 23.31 kB 23.40 kB
facebook-www/ReactServer-dev.modern.js +0.56% 108.25 kB 108.86 kB +0.41% 26.29 kB 26.40 kB
facebook-react-native/react/cjs/React-dev.js +0.48% 127.74 kB 128.34 kB +0.35% 30.89 kB 31.00 kB
oss-stable-semver/react/cjs/react.development.js +0.47% 101.86 kB 102.34 kB +0.33% 27.21 kB 27.30 kB
oss-stable/react/cjs/react.development.js +0.47% 101.89 kB 102.37 kB +0.34% 27.24 kB 27.33 kB
oss-experimental/react/cjs/react.development.js +0.46% 104.42 kB 104.91 kB +0.33% 27.88 kB 27.97 kB
facebook-www/React-dev.modern.js +0.43% 140.57 kB 141.18 kB +0.33% 34.21 kB 34.33 kB
facebook-www/React-dev.classic.js +0.43% 141.79 kB 142.40 kB +0.30% 34.47 kB 34.57 kB
oss-stable-semver/react/umd/react.development.js +0.40% 124.92 kB 125.42 kB +0.29% 31.92 kB 32.01 kB
oss-stable/react/umd/react.development.js +0.40% 124.95 kB 125.44 kB +0.29% 31.95 kB 32.04 kB
oss-experimental/react/umd/react.development.js +0.39% 127.59 kB 128.09 kB +0.32% 32.63 kB 32.73 kB
test_utils/ReactAllWarnings.js Deleted 67.02 kB 0.00 kB Deleted 16.42 kB 0.00 kB

Generated by 🚫 dangerJS against 56ade53

Comment on lines +108 to +112
case REACT_LAZY_TYPE:
throw new Error(
'Cannot render an Async Component, Promise or React.Lazy inside React.Children. ' +
'We recommend not iterating over children and just rendering them plain.',
);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This makes it an error to iterate over Children if any child happens to be one of these things but you may not know that until you iterate over it. Basically it makes using Children risky if you don't control the inputs very carefully. The language here says you can't render but forEach allows you to do arbitrary stuff with the child so perhaps you will omit any non-renderable children.

This feels halfway towards deprecating React.Children but where you may learn about it in painful ways

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

To be clear this is already going to error below so it's not a new change. It's just a better message.

The thing is that you can't omit the lazy because if you did something with an element you should've done it with the unwrapped lazy.

But yea, we've already effectively deprecated React.Children. We're just ambivalent about it.

@sebmarkbage sebmarkbage merged commit e41ee9e into facebook:main Feb 8, 2024
36 checks passed
github-actions bot pushed a commit that referenced this pull request Feb 8, 2024
)

We could in theory actually support this case by throwing a Promise when
it's used inside a render. Allowing it to be synchronously unwrapped.
However, it's a bit sketchy because we officially only support this in
the render's child position or in `use()`.

Another alternative could be to actually pass the Promise/Lazy to the
callback so that you can reason about it and just return it again or
even unwrapping with `use()` - at least for the forEach case maybe.

DiffTrain build for [e41ee9e](e41ee9e)
sebmarkbage added a commit that referenced this pull request Feb 12, 2024
This pains me because `React.Children` is really already
pseudo-deprecated.

`React.Children` takes any children that `React.Node` takes. We now
support Lazy and Thenable in this position elsewhere, but it errors in
`React.Children`.

This becomes an issue with async Server Components which can resolve
into a Lazy and in the future Lazy will just become Thenables. Which
causes this to error.

There are a few different semantics we could have:

1) Error like we already do (#28280). `React.Children` is about
introspecting children. It was always sketchy because you can't
introspect inside an abstraction anyway. With Server Components we fold
away the components so you can actually introspect inside of them kind
of but what they do is an implementation detail and you should be able
to turn it into a Client Component at any point. The type of an Element
passing the boundary actually reduces to `React.Node`.
2) Suspend and unwrap the Node (this PR). If we assume that Children is
called inside of render, then throwing a Promise if it's not already
loaded or unwrapping would treat it as if it wasn't there. Just like if
you rendered it in React. This lets you introspect what's inside which
isn't really something you should be able to do. This isn't compatible
with deprecating throwing-a-Promise and enable static compilation like
`use()` does. We'd have to deprecate `React.Children` before doing that
which we might anyway.
3) Wrap in a Fragment. If a Server Component was instead a Client
Component, you couldn't introspect through it anyway. Another
alternative might be to let it pass through but then it wouldn't be
given a flat key. We could also wrap it in a Fragment that is keyed.
That way you're always seeing an element. The issue with this solution
is that it wouldn't see the key of the Server Component since that gets
forwarded to the child that is yet to resolve. The nice thing about that
strategy is it doesn't depend on throw-a-Promise but it might not be
keyed correctly when things move.
github-actions bot pushed a commit that referenced this pull request Feb 12, 2024
This pains me because `React.Children` is really already
pseudo-deprecated.

`React.Children` takes any children that `React.Node` takes. We now
support Lazy and Thenable in this position elsewhere, but it errors in
`React.Children`.

This becomes an issue with async Server Components which can resolve
into a Lazy and in the future Lazy will just become Thenables. Which
causes this to error.

There are a few different semantics we could have:

1) Error like we already do (#28280). `React.Children` is about
introspecting children. It was always sketchy because you can't
introspect inside an abstraction anyway. With Server Components we fold
away the components so you can actually introspect inside of them kind
of but what they do is an implementation detail and you should be able
to turn it into a Client Component at any point. The type of an Element
passing the boundary actually reduces to `React.Node`.
2) Suspend and unwrap the Node (this PR). If we assume that Children is
called inside of render, then throwing a Promise if it's not already
loaded or unwrapping would treat it as if it wasn't there. Just like if
you rendered it in React. This lets you introspect what's inside which
isn't really something you should be able to do. This isn't compatible
with deprecating throwing-a-Promise and enable static compilation like
`use()` does. We'd have to deprecate `React.Children` before doing that
which we might anyway.
3) Wrap in a Fragment. If a Server Component was instead a Client
Component, you couldn't introspect through it anyway. Another
alternative might be to let it pass through but then it wouldn't be
given a flat key. We could also wrap it in a Fragment that is keyed.
That way you're always seeing an element. The issue with this solution
is that it wouldn't see the key of the Server Component since that gets
forwarded to the child that is yet to resolve. The nice thing about that
strategy is it doesn't depend on throw-a-Promise but it might not be
keyed correctly when things move.

DiffTrain build for [9e7944f](9e7944f)
EdisonVan pushed a commit to EdisonVan/react that referenced this pull request Apr 15, 2024
…ebook#28280)

We could in theory actually support this case by throwing a Promise when
it's used inside a render. Allowing it to be synchronously unwrapped.
However, it's a bit sketchy because we officially only support this in
the render's child position or in `use()`.

Another alternative could be to actually pass the Promise/Lazy to the
callback so that you can reason about it and just return it again or
even unwrapping with `use()` - at least for the forEach case maybe.
EdisonVan pushed a commit to EdisonVan/react that referenced this pull request Apr 15, 2024
…book#28284)

This pains me because `React.Children` is really already
pseudo-deprecated.

`React.Children` takes any children that `React.Node` takes. We now
support Lazy and Thenable in this position elsewhere, but it errors in
`React.Children`.

This becomes an issue with async Server Components which can resolve
into a Lazy and in the future Lazy will just become Thenables. Which
causes this to error.

There are a few different semantics we could have:

1) Error like we already do (facebook#28280). `React.Children` is about
introspecting children. It was always sketchy because you can't
introspect inside an abstraction anyway. With Server Components we fold
away the components so you can actually introspect inside of them kind
of but what they do is an implementation detail and you should be able
to turn it into a Client Component at any point. The type of an Element
passing the boundary actually reduces to `React.Node`.
2) Suspend and unwrap the Node (this PR). If we assume that Children is
called inside of render, then throwing a Promise if it's not already
loaded or unwrapping would treat it as if it wasn't there. Just like if
you rendered it in React. This lets you introspect what's inside which
isn't really something you should be able to do. This isn't compatible
with deprecating throwing-a-Promise and enable static compilation like
`use()` does. We'd have to deprecate `React.Children` before doing that
which we might anyway.
3) Wrap in a Fragment. If a Server Component was instead a Client
Component, you couldn't introspect through it anyway. Another
alternative might be to let it pass through but then it wouldn't be
given a flat key. We could also wrap it in a Fragment that is keyed.
That way you're always seeing an element. The issue with this solution
is that it wouldn't see the key of the Server Component since that gets
forwarded to the child that is yet to resolve. The nice thing about that
strategy is it doesn't depend on throw-a-Promise but it might not be
keyed correctly when things move.
bigfootjon pushed a commit that referenced this pull request Apr 18, 2024
)

We could in theory actually support this case by throwing a Promise when
it's used inside a render. Allowing it to be synchronously unwrapped.
However, it's a bit sketchy because we officially only support this in
the render's child position or in `use()`.

Another alternative could be to actually pass the Promise/Lazy to the
callback so that you can reason about it and just return it again or
even unwrapping with `use()` - at least for the forEach case maybe.

DiffTrain build for commit e41ee9e.
bigfootjon pushed a commit that referenced this pull request Apr 18, 2024
This pains me because `React.Children` is really already
pseudo-deprecated.

`React.Children` takes any children that `React.Node` takes. We now
support Lazy and Thenable in this position elsewhere, but it errors in
`React.Children`.

This becomes an issue with async Server Components which can resolve
into a Lazy and in the future Lazy will just become Thenables. Which
causes this to error.

There are a few different semantics we could have:

1) Error like we already do (#28280). `React.Children` is about
introspecting children. It was always sketchy because you can't
introspect inside an abstraction anyway. With Server Components we fold
away the components so you can actually introspect inside of them kind
of but what they do is an implementation detail and you should be able
to turn it into a Client Component at any point. The type of an Element
passing the boundary actually reduces to `React.Node`.
2) Suspend and unwrap the Node (this PR). If we assume that Children is
called inside of render, then throwing a Promise if it's not already
loaded or unwrapping would treat it as if it wasn't there. Just like if
you rendered it in React. This lets you introspect what's inside which
isn't really something you should be able to do. This isn't compatible
with deprecating throwing-a-Promise and enable static compilation like
`use()` does. We'd have to deprecate `React.Children` before doing that
which we might anyway.
3) Wrap in a Fragment. If a Server Component was instead a Client
Component, you couldn't introspect through it anyway. Another
alternative might be to let it pass through but then it wouldn't be
given a flat key. We could also wrap it in a Fragment that is keyed.
That way you're always seeing an element. The issue with this solution
is that it wouldn't see the key of the Server Component since that gets
forwarded to the child that is yet to resolve. The nice thing about that
strategy is it doesn't depend on throw-a-Promise but it might not be
keyed correctly when things move.

DiffTrain build for commit 9e7944f.
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.

4 participants