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.

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

Support passing children as props to a React component #5886

Merged
merged 1 commit into from
Jan 25, 2023

Conversation

HiDeoo
Copy link
Member

@HiDeoo HiDeoo commented Jan 17, 2023

Changes

This PR fixes #5493.

Passing children as props to a React component, e.g. <WithChildren client:load children="test" /> would break in production with the component disappearing after hydration and React complaining with the following error:

Hydration failed because the initial UI does not match what was rendered on the server.

When creating the static HTML element for children, the props.children props was previously ignored. This PR introduces a change to fallback to props.children if children is not defined which fixes the issue.

This PR also adds this change to packages/integrations/react/server-v17.js for the React 17 integration, altho, while adding this, I noticed that the changes introduced in #4756 were not applied to the React 17 integration so I added them as well.

Testing

I updated the react-component fixture for astro to include a new WithChildren component rendering the children props and updated the associated test to render this component twice, once with children and once with props.children and assert that the rendered HTML is the same (which would fail without the changes in this PR as the <astro-slot> was missing in the second case).

I also manually tested locally this change in a local repro using the file: pnpm protocol and confirmed a component using children passed down as props is now properly rendered in production and the React hydration error is gone.

Docs

I could not find any specific mention regarding this specific behavior in the docs and I'm not sure if it's worth mentioning it.

@changeset-bot
Copy link

changeset-bot bot commented Jan 17, 2023

🦋 Changeset detected

Latest commit: 5f813b6

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 github-actions bot added pkg: astro Related to the core `astro` package (scope) pkg: react Related to React (scope) pkg: integration Related to any renderer integration (scope) labels Jan 17, 2023
@HiDeoo
Copy link
Member Author

HiDeoo commented Jan 17, 2023

Not sure what is going on regarding the CI failure only on macOS as I cannot repro locally (even on macOS) and it looks like it is related to a timeout issue in a Astro CLI related test which should not be impacted by this pull request 😕

@Princesseuh
Copy link
Member

Not sure what is going on regarding the CI failure only on macOS as I cannot repro locally (even on macOS) and it looks like it is related to a timeout issue in a Astro CLI related test which should not be impacted by this pull request 😕

The test is probably just flaky, no worries!

Copy link
Member

@natemoo-re natemoo-re left a comment

Choose a reason for hiding this comment

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

This is a great fix, thank you!

@natemoo-re natemoo-re merged commit 9d4bfc7 into withastro:main Jan 25, 2023
@astrobot-houston astrobot-houston mentioned this pull request Jan 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: astro Related to the core `astro` package (scope) pkg: integration Related to any renderer integration (scope) pkg: react Related to React (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Passing children as props to a react component breaks in production.
3 participants