fix(#15332) use toArray or checks for mapping children #15333
Conversation
dabrad26
changed the title
✅ Deploy Preview for v11-carbon-react ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
I went through and quickly checked all uses of Children.map (https://github.com/search?q=repo%3Acarbon-design-system%2Fcarbon%20React.Children.map&type=code) Quickly looking over each most had checks to validate that children were either Nodes or they had no dangerous code (calling on NULL or cloning). I did find two possible risks with ContentSwitcher and ProgressIndicator and have added them to this PR. |
dabrad26
changed the title
| @@ -56,6 +56,10 @@ function ProgressIndicator({ | |||
| return ( | |||
| <ul className={className} {...rest}> | |||
| {React.Children.map(children, (child, index) => { | |||
| if (!React.isValidElement(child)) { | |||
There was a problem hiding this comment.
This could be written in a few different ways; but I wanted to make as minimal impact; since bottom of this loop returns null I am just adding this one check for valid Element here instead of on each of the index checks below.
There was a problem hiding this comment.
Thanks so much for putting up this fix! Sorry we didn't catch this in the initial PR #14989 😓
I personally find it super difficult to review file changes from .js to .tsx when the git diff splits the file into two separate changes like it did in that PR. If it would've been a unified diff we might've spotted this.
Sorry again for any inconvenience you had from this. Can we cut a new release for this once it's in @guidari @alisonjoseph?
|
Actually we're slated to release 11.45.0 tomorrow, this should go out with that 👍 |
|
Thanks so much everyone for the quick release. Yeah. Typescript migrations are tough. We have done several over the years. So happy to help if you ever want to assign a typescript migration PR to me. :) We could also look to expand unit tests for anything dealing with children to test all possible children (string nodes, null, elements) and that should help. For us our unit tests caught this regression since I didn’t check a Header Bar Search button which returns a null initially. |
|
Hey there! v11.45.0 was just released that references this issue/PR. |
@dabrad26 That would be such a great help, thanks for offering! |
…ldren (carbon-design-system#15333) * fix(carbon-design-system#15332) use toArray for mapping children * fix: Content switcher * fix: progress indicator * fix: typescript rules for ReactElement usage * fix: format diff test
Closes #15332
Using toArray to get rid of null or other non element children. See ticket for more info on how this was changed recently and the benefit of toArray over direct map.