-
Notifications
You must be signed in to change notification settings - Fork 216
feat: support React 19 bigint, and deprecate fragment flattening #4249
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,70 @@ | ||
| // Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
| import React, { Fragment } from 'react'; | ||
|
|
||
| import { flattenChildren } from '../flatten-children'; | ||
|
|
||
| describe('flattenChildren', () => { | ||
| const nestedArrayChildren = [ | ||
| <div key="1">Item 1</div>, | ||
| [<div key="2">Item 2</div>, <div key="3">Item 3</div>], | ||
| <div key="4">Item 4</div>, | ||
| ]; | ||
|
|
||
| const fragmentChildren = [ | ||
| <Fragment key="group"> | ||
| <span key="a">A</span> | ||
| <span key="b">B</span> | ||
| </Fragment>, | ||
| <span key="c">C</span>, | ||
| ]; | ||
|
|
||
| const singleFragment = ( | ||
| <Fragment> | ||
| <span>A</span> | ||
| <span>B</span> | ||
| </Fragment> | ||
| ); | ||
|
|
||
| // Tests React 19+ behavior: uses Children.toArray() which does NOT flatten fragments | ||
| describe('React 19+', () => { | ||
| beforeEach(() => { | ||
| // Mock React.version to trigger Children.toArray() code path | ||
| Object.defineProperty(React, 'version', { | ||
| value: '19.0.0', | ||
| writable: true, | ||
| configurable: true, | ||
| }); | ||
| }); | ||
|
|
||
| it('flattens nested arrays', () => { | ||
| expect(flattenChildren(nestedArrayChildren)).toHaveLength(4); | ||
| }); | ||
|
|
||
| it('does NOT flatten fragments', () => { | ||
| expect(flattenChildren(fragmentChildren)).toHaveLength(2); | ||
| expect(flattenChildren(singleFragment)).toHaveLength(1); | ||
| }); | ||
| }); | ||
|
|
||
| // Tests React 16-18 behavior: uses react-keyed-flatten-children which DOES flatten fragments | ||
| describe('React 16-18', () => { | ||
| beforeEach(() => { | ||
| // Mock React.version to trigger react-keyed-flatten-children code path | ||
| Object.defineProperty(React, 'version', { | ||
| value: '18.2.0', | ||
| writable: true, | ||
| configurable: true, | ||
| }); | ||
| }); | ||
|
|
||
| it('flattens nested arrays', () => { | ||
| expect(flattenChildren(nestedArrayChildren)).toHaveLength(4); | ||
| }); | ||
|
|
||
| it('flattens fragments', () => { | ||
| expect(flattenChildren(fragmentChildren)).toHaveLength(3); | ||
| expect(flattenChildren(singleFragment)).toHaveLength(2); | ||
| }); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| // Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
| import React, { ReactNode } from 'react'; | ||
| import flattenChildrenLegacy from 'react-keyed-flatten-children'; | ||
|
|
||
| export function flattenChildren(children: ReactNode): ReactNode[] { | ||
| const majorVersion = parseInt(React.version.split('.')[0], 10); | ||
|
|
||
| if (majorVersion >= 19) { | ||
| // React 19+: Uses Children.toArray() which doesn't flatten fragments. | ||
| // This also supports bigint which is not available in earlier React versions. | ||
| return React.Children.toArray(children); | ||
| } else { | ||
| // React 16-18: Use react-keyed-flatten-children for backward compatibility | ||
| return flattenChildrenLegacy(children); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,13 +1,13 @@ | ||
| // Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
| import React, { forwardRef } from 'react'; | ||
| import flattenChildren from 'react-keyed-flatten-children'; | ||
| import clsx from 'clsx'; | ||
|
|
||
| import { useMergeRefs } from '@cloudscape-design/component-toolkit/internal'; | ||
|
|
||
| import { getBaseProps } from '../internal/base-component'; | ||
| import { InternalBaseComponentProps } from '../internal/hooks/use-base-component'; | ||
| import { flattenChildren } from '../internal/utils/flatten-children'; | ||
| import WithNativeAttributes from '../internal/utils/with-native-attributes'; | ||
| import { SpaceBetweenProps } from './interfaces'; | ||
|
|
||
|
|
@@ -52,10 +52,11 @@ const InternalSpaceBetween = forwardRef( | |
| ref={mergedRef} | ||
| > | ||
| {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; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And what about Before the changes, the code was safe because
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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: 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? |
||
|
|
||
| return ( | ||
| <div key={key} className={styles.child}> | ||
| <div key={key ? String(key) : undefined} className={styles.child}> | ||
| {child} | ||
| </div> | ||
| ); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since
React.versionis 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, I will do it like that.