Skip to content

feat: add test to reproduce #605 #1

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

Merged

Conversation

stephenkilbourn
Copy link
Owner

@stephenkilbourn stephenkilbourn commented Nov 14, 2022

What:

This fixes a bug that assumes that tests don't run in SSR environments.

Why:

With frameworks like Next.js and Remix rendering components on the server, it's important that this library can mimic SSR environments as closely as possible.

How:

First by adding a test that reproduces the issue and then by moving this logic "into the hydrate function, so we reference document when they want it to render for real." (suggested by mpeyper).

Checklist:

  • Documentation updated
  • Tests
  • Ready to be merged
  • Added myself to contributors table

act(() => {
ReactDOM.hydrate(testHarness(renderProps), container)
ReactDOM.hydrate(testHarness(renderProps), container || null)
Copy link
Owner Author

Choose a reason for hiding this comment

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

✍🏼 If container is undefined, then we pass null to hydrate because undefined is not a valid container, but null is

act(() => {
ReactDOM.unmountComponentAtNode(container)
if (typeof container !== 'undefined') {
Copy link
Owner Author

Choose a reason for hiding this comment

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

✍🏼 Because act returns a function, it introduces a new scope which means we need to check again that container is not undefined before using it. This means we could remove the outer if (container) { check if we're okay that unmount always returns act}

@stephenkilbourn stephenkilbourn merged commit 309949a into stephenkilbourn--fix-ssr-before Nov 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant