Skip to content

fix: update "children" type #6648

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
Merged
merged 2 commits into from
Mar 1, 2023
Merged

fix: update "children" type #6648

merged 2 commits into from
Mar 1, 2023

Conversation

vgpena
Copy link
Contributor

@vgpena vgpena commented Feb 22, 2023

What does this PR do?

This PR updates the children type of the Checkbox to include React.ReactNode. This fixes a regression introduced when bringing in explicit typings for React Children. It also makes Checkbox consistent with the other changes in #6066 and with React's PropsWithChildren type signature (link).

Where should the reviewer start?

Compare the type update here to RadioButton > children and React's PropsWithChildren.

What testing has been done on this PR?

  • tested locally in a linked environment
  • manual testing steps (see below)

How should this be manually tested?

  1. On the master branch, rename Checkbox/stories/Children.js to Checkbox/stories/Children.tsx.
  2. On lines 17-19 of Checkbox/stories/Children.tsx, substitute the function child component with a plain React.ReactNode. (e.g., <span>foo</span>). You should get a TypeScript error.
  3. Stash that change, switch to this branch, and apply the change (or some equivalent -- we want the updated story file and the updated type definition on the same branch). You should no longer see a TypeScript error in Checkbox/stories/Children.tsx.

Do Jest tests follow these best practices?

  • screen is used for querying.
  • The correct query is used. (Refer to this list of queries)
  • userEvent is used in place of fireEvent.
  • asFragment() is used for snapshot testing.

Any background context you want to provide?

This issue was discovered because of a project that consumes Grommet and uses React's PropsWithChildren type.

What are the relevant issues?

N/A

Screenshots (if appropriate)

N/A

Do the grommet docs need to be updated?

N/A

Should this PR be mentioned in the release notes?

Yes

Is this change backwards compatible or is it a breaking change?

Backwards-compatible

@jcfilben
Copy link
Collaborator

Hey @vgpena I looked a bit more at the change I made to the RadioButton children type in the React 18 pull request and I think that I shouldn't have set children?: React.ReactNode | Function; and it should just be children?: Function;. I thought about this more and couldn't think of a case where we would want to pass a react node. In addition, the documentation for RadioButton and CheckBox the children prop only show function as a valid type to pass.

Could you share the use case for wanting to pass a react node? Maybe there is a valid case that I am just not thinking of at the moment.

@vgpena
Copy link
Contributor Author

vgpena commented Feb 28, 2023

Hi @jcfilben , I see the point you're making, and the primary use case definitely seems to be using a function instead of a React.ReactNode.

The actual issue I'm trying to solve is about backwards compatibility. With implicit React children, React.ReactNode is allowed to be passed in. The React 18 PR in Grommet removes support for React.ReactNode as a child of the Checkbox. From the perspective of an application that consumes Grommet and uses TypeScript, this is a breaking API change. In our case since we're using React's PropsWithChildren type (link), upgrading past v2.23 of Grommet requires code changes on our end which is normally only required for major-version upgrades.

The official recommendation of how to add explicit child types is here (this ignores functions but in some testing locally it seems like React.ReactElement allows for () => React.ReactElement as well).

Within Grommet, looking at the children type in RadioButton link is React.ReactNode | Function, which made me think that Checkbox should be the same and the lack of React.ReactNode was a transcription error.

Hope this clears things up!

@jcfilben
Copy link
Collaborator

jcfilben commented Mar 1, 2023

Appreciate the thoughtful response and links included, very helpful! Looking at it from the perspective of backwards compatibility I think your argument makes sense I am good with moving forward with the changes proposed in this PR.

jcfilben
jcfilben approved these changes Mar 1, 2023
View reviewed changes
Copy link
Collaborator

@jcfilben jcfilben left a comment

Choose a reason for hiding this comment

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

Looks good!

@jcfilben jcfilben requested a review from ericsoderberghp March 1, 2023 20:18
@ericsoderberghp ericsoderberghp merged commit 3af3a54 into grommet:master Mar 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jcfilben jcfilben jcfilben approved these changes

+1 more reviewer

@ericsoderberghp ericsoderberghp ericsoderberghp approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@vgpena @jcfilben @ericsoderberghp