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

Context System: Don't explicitly set undefined value to children #42686

Merged
merged 6 commits into from
Jul 26, 2022
Merged

Context System: Don't explicitly set undefined value to children #42686

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9bfe6c7
Add tests
mirka Jul 25, 2022
0251e42
Simplify tests
mirka Jul 25, 2022
cc107e4
Compare with normal component behavior
mirka Jul 25, 2022
9ed8fdf
Components: Allow rendering `Icon` via `as` prop
mirka Jul 25, 2022
c5bb58a
Add changelog
mirka Jul 25, 2022
2b798b9
Tweak changelog
mirka Jul 26, 2022
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
1 change: 1 addition & 0 deletions packages/components/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

### Bug Fix

- Allow rendering `Icon` via `as` prop ([#42686](https://github.com/WordPress/gutenberg/pull/42686)).
mirka marked this conversation as resolved.
Show resolved Hide resolved
- `Popover`, `Dropdown`: Fix width when `expandOnMobile` is enabled ([#42635](https://github.com/WordPress/gutenberg/pull/42635/)).
- `CustomSelectControl`: Fix font size and hover/focus style inconsistencies with `SelectControl` ([#42460](https://github.com/WordPress/gutenberg/pull/42460/)).
- `AnglePickerControl`: Fix gap between elements in RTL mode ([#42534](https://github.com/WordPress/gutenberg/pull/42534)).
Expand Down
102 changes: 101 additions & 1 deletion packages/components/src/ui/context/test/context-system-provider.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,14 @@
/**
* External dependencies
*/
import { render } from '@testing-library/react';
import { render, screen } from '@testing-library/react';
import styled from '@emotion/styled';

/**
* WordPress dependencies
*/
import { cloneElement } from '@wordpress/element';

/**
* Internal dependencies
*/
Expand Down Expand Up @@ -101,3 +106,98 @@ describe( 'props', () => {
expect( el.innerHTML ).not.toContain( 'WordPress.org' );
} );
} );

describe( 'children', () => {
test( 'should pass through children', () => {
const Component = ( props, ref ) => (
<View { ...useContextSystem( props, 'Component' ) } ref={ ref } />
);
const ConnectedComponent = contextConnect( Component, 'Component' );

render(
<ContextSystemProvider>
<ConnectedComponent>Pass through</ConnectedComponent>
</ContextSystemProvider>
);

expect( screen.getByText( 'Pass through' ) ).toBeInTheDocument();
} );

test( 'should not accept children via `context`', () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

I actually don't know if this is intentional/desirable, but it is the existing behavior in trunk.

const Component = ( props, ref ) => (
<View { ...useContextSystem( props, 'Component' ) } ref={ ref } />
);
const ConnectedComponent = contextConnect( Component, 'Component' );

render(
<ContextSystemProvider
context={ { Component: { children: 'Override' } } }
>
<ConnectedComponent />
</ContextSystemProvider>
);

expect( screen.queryByText( 'Override' ) ).not.toBeInTheDocument();
} );

// This matches the behavior for normal, non-context-connected components.
test( 'should not override inherent children', () => {
const Component = ( props, ref ) => (
<View { ...useContextSystem( props, 'Component' ) } ref={ ref }>
Inherent
</View>
);
const ConnectedComponent = contextConnect( Component, 'Component' );
const NormalComponent = ( props ) => <div { ...props }>Inherent</div>;

render(
<ContextSystemProvider>
<ConnectedComponent />
Copy link
Member Author

Choose a reason for hiding this comment

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

This test (passing on trunk) establishes how implicitly undefined children were not a problem as long as the underlying component did not do a cloneElement().

<ConnectedComponent>Explicit children</ConnectedComponent>
<NormalComponent />
<NormalComponent>Explicit children</NormalComponent>
</ContextSystemProvider>
);

expect( screen.getAllByText( 'Inherent' ) ).toHaveLength( 4 );
} );

describe( 'when connected component does a `cloneElement()`', () => {
// eslint-disable-next-line no-unused-vars
const ComponentThatClones = ( { content, ...props }, _ref ) =>
cloneElement(
content,
useContextSystem( props, 'ComponentThatClones' )
);
const ConnectedComponentThatClones = contextConnect(
ComponentThatClones,
'ComponentThatClones'
);

test( 'should not override cloned inherent children with implicit `undefined` children', () => {
render(
<ContextSystemProvider>
<ConnectedComponentThatClones
content={ <span>Inherent</span> }
/>
</ContextSystemProvider>
);
expect( screen.getByText( 'Inherent' ) ).toBeInTheDocument();
} );

test( 'should override cloned inherent children with explicit children', () => {
render(
<ContextSystemProvider>
<ConnectedComponentThatClones
content={ <span>Inherent</span> }
>
Explicit children
</ConnectedComponentThatClones>
</ContextSystemProvider>
);
expect(
screen.getByText( 'Explicit children' )
).toBeInTheDocument();
} );
} );
} );
9 changes: 7 additions & 2 deletions packages/components/src/ui/context/use-context-system.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,8 +71,13 @@ export function useContextSystem( props, namespace ) {
finalComponentProps[ key ] = overrideProps[ key ];
}

// @ts-ignore
finalComponentProps.children = rendered;
// Setting an `undefined` explicitly can cause unintended overwrites
// when a `cloneElement()` is involved.
if ( rendered !== undefined ) {
// @ts-ignore
finalComponentProps.children = rendered;
}
Comment on lines +74 to +79
Copy link
Member Author

Choose a reason for hiding this comment

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

In the very unlikely case that someone wants to explicitly wipe out the inherent children of a component using cloneElement(), they can still pass children=null.

For normal components that don't involve cloneElement(), children overrides will be (and have been) ignored anyway.


finalComponentProps.className = classes;

return finalComponentProps;
Expand Down