feat: support React 19 bigint, and deprecate fragment flattening#4249
feat: support React 19 bigint, and deprecate fragment flattening#4249
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4249 +/- ##
=======================================
Coverage 97.19% 97.19%
=======================================
Files 886 887 +1
Lines 26021 26038 +17
Branches 9433 9439 +6
=======================================
+ Hits 25291 25308 +17
Misses 724 724
Partials 6 6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
7211bc0 to
6f56617
Compare
| import flattenChildrenLegacy from 'react-keyed-flatten-children'; | ||
|
|
||
| export function flattenChildren(children: ReactNode): ReactNode[] { | ||
| const majorVersion = parseInt(React.version.split('.')[0], 10); |
There was a problem hiding this comment.
Since React.version is undocumented, should we make this line more failproof? And if checking the version fails, then fall back to the new behavior (i.e, not handling fragments, same as version >= 19)
There was a problem hiding this comment.
Good point, I will do it like that.
| {flattenedChildren.map(child => { | ||
| const key = typeof child === 'object' ? child.key : undefined; | ||
| // If this react child is a primitive value, the key will be undefined | ||
| const key = (child as Record<'key', unknown>).key; |
There was a problem hiding this comment.
Is there a possible case where this will throw an error? Let's say some conditional rendering logic where a child can be rendered as undefined or a number.
There was a problem hiding this comment.
if the child is a number, bigint (in react 19), or string the key will be undefined. so we will be assigning undefined key. But it shouldn't throw an error.
There was a problem hiding this comment.
And what about undefined?
Before the changes, the code was safe because typeof undefined is not 'object', therefore we render undefined according to the ternary operator, but after the changes, we will try to access a property of undefined, which throws an error.
There was a problem hiding this comment.
You're right that accessing a property on undefined would throw an error. However, this is safe because both Children.toArray() and react-keyed-flatten-children filter out undefined, null, and boolean values.
Looking at the type signature of toArray it returns the following: toArray(children: ReactNode | ReactNode[]): Array<Exclude<ReactNode, boolean | null | undefined>>; The return type explicitly excludes undefined, null, and boolean. React's Children.toArray() implementation (test example). Under the hood, react-keyed-flatten-children also uses Children.toArray so they are filtered automatically, in both cases.
Here is another place where we also use similar code (line 85). That said, if you'd prefer more safe approach, we could add a guard and/or I can change the other implementation as well. What do you think?
Description
Cloudscape components (ColumnLayout, Grid, and SpaceBetween) currently flatten React Fragments using react-keyed-flatten-children (v2.2.1), which depends on react-is (^18.2.0). React 19 renamed internal type elements, breaking fragment detection and flattening when using older versions of react-is.
Changes
Tested by
Related links, issue #, if available: PptYApl47Z5P
How has this been tested?
Review checklist
The following items are to be evaluated by the author(s) and the reviewer(s).
Correctness
CONTRIBUTING.md.CONTRIBUTING.md.Security
checkSafeUrlfunction.Testing
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.