Add PropsWithChildren type for components with children

[jonkoops]

Adds a type PropsWithChildren around all components that have children. In React 18 a component type (e.g. FunctionComponent) no longer has an implicit children prop. Adding the PropsWithChildren type restores this behavior.

This is needed to land #7142.

[patternfly-build]

Preview: https://patternfly-react-pr-7219.surge.sh

A11y report: https://patternfly-react-pr-7219-a11y.surge.sh

[jonkoops]

Can we get some eyes on this? I'll have to keep rebasing and fixing up this PR for most merges upstream.

Note these changes are generated by means of a codemod, so if you'd like to make these changes yourself to verify there are no malicious changes you can run:

npx types-react-codemod implicit-children .
yarn lint packages --fix

[jenny-s51]

Hi @jonkoops, thank you for these updates!

I'm noticing that for all of the components declared with React.FunctionComponent<React.PropsWithChildren<unknown>>, the react demo for that component doesn't render and displays SyntaxError: Unexpected token. Do you have any idea why this might be happening?

Other demos look great and seem to render fine 👍

[jonkoops]

@redallen Perhaps you know how this works? I feel like the TypeScript code is stripped out by theme-patternfly-org somehow, but it trips up when a generic argument is provided?

[nicolethoen]

Unfortunately, redallen is not supporting this project anymore - but maybe we could have @wise-king-sullyman, @jschuler, @jeffpuzzo, or @dgutride take a peek

[wise-king-sullyman]

I don't have extensive knowledge of org, but I can take a look into this today.

A potentially silly question though: since the standard PF pattern is (at least as far as I'm aware) to declare an explicit children prop on the component interface when we have a component take children, is this PR needed?

[jschuler]

@wise-king-sullyman you raise a good point, how about we just make sure that all components that currently depend on implicit children add children to its props? (if there are any such components)
@jonkoops

[jonkoops]

Since the standard PF pattern is (at least as far as I'm aware) to declare an explicit children prop on the component interface when we have a component take children, is this PR needed?

@wise-king-sullyman I tried doing an upgrade to the React 18 types and there were quite some places where FunctionComponent was being used without explicitly adding the children prop.

It does look however that this migration is applied everywhere where FunctionComponent is used, even if such a component does not actually expose any children. This is (I assume) to preserve backwards compatibility with existing consumers, as they might be passing children even though nothing happens with them.

[wise-king-sullyman]

This is (I assume) to preserve backwards compatibility with existing consumers, as they might be passing children even though nothing happens with them.

@jonkoops that's true, going the direction of only having children on component interfaces where those children are actually used could be breaking for some consumers. Though as you said it should only be on consumers that are passing children to components that don't take them, so I'm not positive if that would be a blocker or not.

If so it seems like we would need to proceed with this PR, at least until we can do a breaking change.

[jschuler]

IMO if a consumer uses a component that does not take children, but they still pass them, it's beneficial to "break" them since they might have the wrong assumption that something should be rendered even though it isn't.
@jonkoops do you know if passing empty child would cause an issue? like <Radio isChecked label="Label"></Radio>

[jpuzz0]

IMO if a consumer uses a component that does not take children, but they still pass them, it's beneficial to "break" them since they might have the wrong assumption that something should be rendered even though it isn't. @jonkoops do you know if passing empty child would cause an issue? like <Radio isChecked label="Label"></Radio>

I agree with @jschuler. It's an opportunity to vet these components and if children are optional or required to specify the children prop explicitly as such. If components allow for children and shouldn't, let's correct those props and remove children.

[jonkoops]

do you know if passing empty child would cause an issue? like <Radio isChecked label="Label"></Radio>

This should not result in having any children passed, at least not from a TypeScript perspective. I believe in the case for React it would simply provide undefined as a value, which is expected behavior.

I am a bit hesitant to commit changes that can cause errors in TypeScript, even when the consumer is wrong. For example, I contributed a breaking type change that resulted in (correct) type errors which was reverted (#6766 (comment)).

Personally I agree with @jschuler and @jeffpuzzo but I'd like to have some confirmation before I manually check all of these instances, which is a significant workload.

[tlabaj]

IMO if a consumer uses a component that does not take children, but they still pass them, it's beneficial to "break" them since they might have the wrong assumption that something should be rendered even though it isn't. @jonkoops do you know if passing empty child would cause an issue? like <Radio isChecked label="Label"></Radio>

I agree with @jschuler. It's an opportunity to vet these components and if children are optional or required to specify the children prop explicitly as such. If the consumer is passing a components allow for children and shouldn't, let's correct those props and remove children.

I would agree with @jschuler and @jeffpuzzo here.
@jonkoops I understand your hesitation. I do however think this is a little different from your PR we had to revert for Tabs. In the case where a consumer would be passing a children prop that is not explicitly declared, our component would not render it. So technically they are no using the component correctly. I think that might be a good check for the consumer. For the Tabs issue, there was a corner case where it actually did break the api.

We should update any of our components that are not explicitly declaring children that expect the prop. We can open an issue for this and take on the work on our end and split it up with you.

[jonkoops]

Awesome, then I think we have reached a conclusion on the approach. I'll go ahead and close this PR so we can instead focus on some smaller PRs that will update the types for individual components where necessary.