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

feat(server): add SSR test for renderHook #799

Merged
merged 5 commits into from Jun 18, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion .all-contributorsrc
Expand Up @@ -145,7 +145,8 @@
"avatar_url": "https://avatars3.githubusercontent.com/u/3806031?v=4",
"profile": "https://jsjoe.io",
"contributions": [
"tutorial"
"tutorial",
"test"
]
},
{
Expand Down
2 changes: 1 addition & 1 deletion README.md
Expand Up @@ -198,7 +198,7 @@ Thanks goes to these wonderful people ([emoji key](https://allcontributors.org/d
<td align="center"><a href="https://github.com/VinceMalone"><img src="https://avatars0.githubusercontent.com/u/2516349?v=4?s=100" width="100px;" alt=""/><br /><sub><b>Vince Malone</b></sub></a><br /><a href="https://github.com/testing-library/react-hooks-testing-library/commits?author=VinceMalone" title="Code">💻</a></td>
<td align="center"><a href="https://github.com/doppelmutzi"><img src="https://avatars1.githubusercontent.com/u/4130968?v=4?s=100" width="100px;" alt=""/><br /><sub><b>Sebastian Weber</b></sub></a><br /><a href="#blog-doppelmutzi" title="Blogposts">📝</a></td>
<td align="center"><a href="https://gillchristian.xyz"><img src="https://avatars2.githubusercontent.com/u/8309423?v=4?s=100" width="100px;" alt=""/><br /><sub><b>Christian Gill</b></sub></a><br /><a href="https://github.com/testing-library/react-hooks-testing-library/commits?author=gillchristian" title="Documentation">📖</a></td>
<td align="center"><a href="https://jsjoe.io"><img src="https://avatars3.githubusercontent.com/u/3806031?v=4?s=100" width="100px;" alt=""/><br /><sub><b>JavaScript Joe</b></sub></a><br /><a href="#tutorial-jsjoeio" title="Tutorials">✅</a></td>
<td align="center"><a href="https://jsjoe.io"><img src="https://avatars3.githubusercontent.com/u/3806031?v=4?s=100" width="100px;" alt=""/><br /><sub><b>JavaScript Joe</b></sub></a><br /><a href="#tutorial-jsjoeio" title="Tutorials">✅</a> <a href="https://github.com/testing-library/react-hooks-testing-library/commits?author=jsjoeio" title="Tests">⚠️</a></td>
</tr>
<tr>
<td align="center"><a href="http://frontstuff.io"><img src="https://avatars1.githubusercontent.com/u/5370675?v=4?s=100" width="100px;" alt=""/><br /><sub><b>Sarah Dayan</b></sub></a><br /><a href="#platform-sarahdayan" title="Packaging/porting to new platform">📦</a></td>
Expand Down
18 changes: 18 additions & 0 deletions src/__tests__/ssr.test.ts
@@ -0,0 +1,18 @@
/**
* @jest-environment node
*/
Comment on lines +1 to +3
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would be nice if we could have sone server only tests run in the node environment without jsdom to make sure we don't break it in the future, but I'm not sure how we would do that for just some tests?

✍🏼 This technique is handy for running specific tests in a Node environment.

See docs.

import { useState } from 'react'

// This verifies that renderHook can be called in
// a SSR-like environment.
describe('renderHook', () => {
function useLoading() {
const [loading, setLoading] = useState(false)
return { loading, setLoading }
}
runForRenderers(['server'], ({ renderHook }) => {
test('should not throw in SSR environment', () => {
expect(() => renderHook(() => useLoading())).not.toThrowError('document is not defined')
})
})
})
14 changes: 6 additions & 8 deletions src/server/pure.ts
Expand Up @@ -13,19 +13,17 @@ function createServerRenderer<TProps, TResult>(
) {
let renderProps: TProps | undefined
let container: HTMLDivElement | undefined
let serverOutput: string = ''
let serverOutput = ''
Copy link
Contributor Author

Choose a reason for hiding this comment

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

🧹 Since we give serverOutput a default value of '', TS can infer string type. The type annotation here is not needed.

const testHarness = createTestHarness(rendererProps, wrapper, false)

return {
Copy link
Member

Choose a reason for hiding this comment

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

github won't let me comment on the actual line, but the error in the test is being caused by the act call on line 22. For some reason, when you use react-dom@16.9.0 this tried to use the document which throws an error when testing in the node environment, which doesn't occur in newer versions.

You can replicate this by running:

npm run install:react-16.9.0
npm test -- --no-watch src/__tests__/ssr.test.ts

In the context of server rendering, the act call is not really necessary here. Dropping it out still passes all of our tests and allows your new test to pass with the older version:

    render(props?: TProps) {
      renderProps = props
      try {
        serverOutput = ReactDOMServer.renderToString(testHarness(props))
      } catch (e: unknown) {
        rendererProps.setError(e as Error)
      }
    },

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wow, I don't think I would have figured that out on my own 😅 I'll test this out and make the necessary changes. Thanks a bunch!

render(props?: TProps) {
renderProps = props
act(() => {
try {
serverOutput = ReactDOMServer.renderToString(testHarness(props))
} catch (e: unknown) {
rendererProps.setError(e as Error)
}
})
try {
serverOutput = ReactDOMServer.renderToString(testHarness(props))
} catch (e: unknown) {
rendererProps.setError(e as Error)
}
},
hydrate() {
if (container) {
Expand Down