Fix Issues with passing void elements to React with experimentalReactChildren flag set
#9849
Conversation
🦋 Changeset detectedLatest commit: 611bed3 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
github-actions
bot
added
pkg: example
| }, {}); | ||
| const className = innerProps.class; | ||
| delete innerProps.class; | ||
| const isVoidElement = ['img', 'input', 'br', 'hr', 'meta', 'area', 'base', 'col', 'command', 'embed', 'keygen', 'link', 'param', 'source', 'track', 'wbr'].includes(node.nodeName.toLowerCase()); |
There was a problem hiding this comment.
This is perfect. On the happy path, everything will be smooth sailing. It leaves no room for the react error to appear.
However, in case the user doesn't know that react makes a distinction between normal and void elements, or if they dont know about them, it could lead to confusion. Our code would (correctly) eat up elements, but the user would just find that they silently disappeared.
I think we should be less smart here and make children undefined anytime element.childNodes has a length of 0. It might even be a lot less code for maintainers to review, which is a bonus.
There was a problem hiding this comment.
I've gone back to the children:undefined method and updated the tests to match this. I don't think I'm understanding when you say Our code would (correctly) eat up elements, but the user would just find that they silently disappeared. Void elements can't have children, so do we want an error to appear if you try to pass children to a void element? Or we could move that child outside of the image elements.
There was a problem hiding this comment.
do we want an error to appear if you try to pass children to a void element?
Yes, that's the idea. Astro shouldn't be autocorrecting on behalf of the user.
There was a problem hiding this comment.
Hey I found a much simpler approach to fix the void elements. It seems to throw errors when passing children to void elements but also still renders the child as a separate element and doesn't fully break the hydration
There was a problem hiding this comment.
Also is there any future thought about making the ReactChildrenFlag component based so users can decide whether to enable it based on their component's needs rather than turning it on for the whole project.
There was a problem hiding this comment.
It hasn't come up before. Do you have just one component that needs the slotted elements to be parsed?
There was a problem hiding this comment.
Well normally you don't need it for every react component. My specific usecase I needed it for an Image carousel component. The docs say that the flag lowers performance (although I'm not sure how much) so I figure if the flag could be changed to work like a prop where each component can decide to parse elements then that would help with performance.
github-actions
bot
added
feat: markdown
github-actions
bot
removed
feat: markdown
github-actions
bot
removed
pkg: create-astro
|
Sorry, I meant to ask other maintainers for review but it was the weekend and I'm just remembering. |
|
Great work, @StandardGage! New codebases can be intimidating but you somehow dialed in on the perfect spot to improve it! 🎯 |
Changes
Allows void elements such as images to be passed as children and be hydrated when using experimentalReactChildren.
should close #9833
Testing
Testing was updated to check that void elements do not have a child property and do not have dangerouslySetInnerHTML. Also updated react example to test and make showcase of the images working with a simple carousel.
Docs
https://docs.astro.build/en/guides/integrations-guide/react/#children-parsing
The docs make it seem as if this feature should already work, so I don't suppose docs would need to be updated. However, they could be updated to specifically show it working.