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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 1 commit into from
Feb 8, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions packages/react/src/ReactChildren.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import isArray from 'shared/isArray';
import {
getIteratorFn,
REACT_ELEMENT_TYPE,
REACT_LAZY_TYPE,
REACT_PORTAL_TYPE,
} from 'shared/ReactSymbols';
import {checkKeyStringCoercion} from 'shared/CheckStringCoercion';
Expand Down Expand Up @@ -103,6 +104,12 @@ function mapIntoArray(
case REACT_ELEMENT_TYPE:
case REACT_PORTAL_TYPE:
invokeCallback = true;
break;
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.',
);
Comment on lines +108 to +112
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.

}
}
}
Expand Down Expand Up @@ -207,6 +214,13 @@ function mapIntoArray(
// eslint-disable-next-line react-internal/safe-string-coercion
const childrenString = String((children: any));

if (typeof (children: any).then === 'function') {
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.',
);
}

throw new Error(
`Objects are not valid as a React child (found: ${
childrenString === '[object Object]'
Expand Down
22 changes: 22 additions & 0 deletions packages/react/src/__tests__/ReactChildren-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -948,6 +948,28 @@ describe('ReactChildren', () => {
);
});

it('should throw on React.lazy', async () => {
const lazyElement = React.lazy(async () => ({default: <div />}));
await expect(() => {
React.Children.forEach([lazyElement], () => {}, null);
}).toThrowError(
'Cannot render an Async Component, Promise or React.Lazy inside React.Children. ' +
'We recommend not iterating over children and just rendering them plain.',
{withoutStack: true}, // There's nothing on the stack
);
});

it('should throw on Promises', async () => {
const promise = Promise.resolve(<div />);
await expect(() => {
React.Children.forEach([promise], () => {}, null);
}).toThrowError(
'Cannot render an Async Component, Promise or React.Lazy inside React.Children. ' +
'We recommend not iterating over children and just rendering them plain.',
{withoutStack: true}, // There's nothing on the stack
);
});

it('should throw on regex', () => {
// Really, we care about dates (#4840) but those have nondeterministic
// serialization (timezones) so let's test a regex instead:
Expand Down
3 changes: 2 additions & 1 deletion scripts/error-codes/codes.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -489,5 +489,6 @@
"501": "The render was aborted with postpone when the shell is incomplete. Reason: %s",
"502": "Cannot read a Client Context from a Server Component.",
"503": "Cannot use() an already resolved Client Reference.",
"504": "Failed to read a RSC payload created by a development version of React on the server while using a production version on the client. Always use matching versions on the server and the client."
"504": "Failed to read a RSC payload created by a development version of React on the server while using a production version on the client. Always use matching versions on the server and the client.",
"505": "Cannot render an Async Component, Promise or React.Lazy inside React.Children. We recommend not iterating over children and just rendering them plain."
}