feat(server): add SSR test for renderHook #799
Conversation
This adds a new test to verify that renderHook can be called in an SSR-like environment based on the changes implemented in testing-library#607.
❌ Deploy Preview for react-hooks-testing-library failed.
|
Codecov Report
@@ Coverage Diff @@
## main #799 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 15 15
Lines 246 245 -1
Branches 34 34
=========================================
- Hits 246 245 -1
Continue to review full report at Codecov.
|
| /** | ||
| * @jest-environment node | ||
| */ |
There was a problem hiding this comment.
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.
| @@ -13,7 +13,7 @@ function createServerRenderer<TProps, TResult>( | |||
| ) { | |||
| let renderProps: TProps | undefined | |||
| let container: HTMLDivElement | undefined | |||
| let serverOutput: string = '' | |||
| let serverOutput = '' | |||
There was a problem hiding this comment.
🧹 Since we give serverOutput a default value of '', TS can infer string type. The type annotation here is not needed.
I looked at |
|
Hey @mpeyper - looks like the test is failing. Would love to get your insights to fix this 🙏 |
|
The test looks fine, I’ll see if I can get to the bottom of it. |
| @@ -13,7 +13,7 @@ function createServerRenderer<TProps, TResult>( | |||
| ) { | |||
| let renderProps: TProps | undefined | |||
| let container: HTMLDivElement | undefined | |||
| let serverOutput: string = '' | |||
| let serverOutput = '' | |||
| const testHarness = createTestHarness(rendererProps, wrapper, false) | |||
|
|
|||
| return { | |||
There was a problem hiding this comment.
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 [email protected] 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.tsIn 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)
}
},There was a problem hiding this comment.
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!
@mpeyper explained how this `act` call in server rendering is not really necessary so we can remove it.
|
Well, that worked! Of course now Netlify is failing lol 12:34:31 PM: error Error in "/opt/build/repo/node_modules/gatsby-plugin-mdx/gatsby-node.js": globalThis is not defined |
|
Yeah, the Netlify build had been failing for a while and I’m not sure how to fix it. I’ve mostly been ignoring it with maintenance on this library wrapping up soon anyway. This all looks fine to me and will try to get it merged next time I’m at my computer. |
|
@mpeyper any updates on this by chance? 👀 |
|
Sorry for the delay, and thanks again for the effort. |
|
🎉 This PR is included in version 8.0.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
woohoo! thanls for letting me bug you and for the tips! |
What:
This PR adds a new test based on the work done in #607 to fix #605. Partially related to #796
Why:
I noticed a test wasn't added when that bug was fixed. I decided to add a test so we could prevent regressions.
How:
A new test was added which runs in a Node environment in Jest.
Checklist: