Better error handling for hydrate in non-browser environment

[jsjoeio]

• react-hooks-testing-library version: v8.0.0-alpha.1 - created a commit starting here
• react version: N/A
• react-dom version (if applicable): N/A
• react-test-renderer version (if applicable): N/A
• node version: v14.17.6
• npm (or yarn) version: v1.22.17

Relevant code or config:

See below

What you did:

I added a test that runs in a Node environment to see if #605 was fixed (sidenote: using this issue/solution in a TypeScript course I'm authoring).

What happened:

I thought the solution would allow renderHook to be called in SSR environments but we get the same issue.

Summary of all failing tests
 FAIL  src/server/__tests__/ssr.test.ts
  ● ssr › should not throw ReferenceError

    The error below may be caused by using the wrong test environment, see https://jestjs.io/docs/en/configuration#testenvironment-string.
    Consider using the "jsdom" test environment.
    
    ReferenceError: document is not defined

      32 |         throw new Error('The component can only be hydrated once')
      33 |       } else {
    > 34 |         container = document.createElement('div')
         |         ^
      35 |         container.innerHTML = serverOutput
      36 |         act(() => {
      37 |           ReactDOM.hydrate(testHarness(renderProps), container!)

      at hydrate (src/server/pure.ts:34:9)
      at Object.<anonymous> (src/server/__tests__/ssr.test.ts:20:5)


Test Suites: 1 failed, 61 passed, 62 total
Tests:       1 failed, 204 passed, 205 total
Snapshots:   0 total
Time:        7.859 s
Ran all test suites.

Note: I added the original author on Discord to try and contact them about this PR. I saw in the comments they said they tested in a Node environment so I may be doing something differently.

Reproduction:

1. git clone https://github.com/testing-library/react-hooks-testing-library.git
2. touch src/server/__tests__/ssr.test.ts
3. Add this code:

/**
 * @jest-environment node
 */
import { useState } from 'react'
import { renderHook, act } from '..'

// This verifies that renderHook can be called in
// a SSR-like environment.
describe('ssr', () => {
  function useLoading() {
    if (typeof window !== 'undefined') {
      const [loading, setLoading] = useState(false)
      return { loading, setLoading }
    }
  }

  test('should not throw ReferenceError', () => {
    const { result, hydrate } = renderHook(() => useLoading())

    hydrate()

    act(() => result?.current?.setLoading(true))

    expect(result?.current?.loading).toBe(true)
  })
})

4. Run yarn test --watch=false

Problem description:

Even though #607 modified src/server/pure.ts to fill the container with the ReactDOM tree in hydrate(), this still assumes document is always available.

Suggested solution:

I see three potential solutions.

Option 1: add DOM to test before hydrate

Based on my understanding, hydrate is supposed to be called on the client. So in our test, we could create a mock document and make sure it's available globally before calling hydrate. I see this more like a bandaid though.

Option 2: check if document is defined

Add a check to see if document is available before using it.

    hydrate() {
      if (container) {
        throw new Error('The component can only be hydrated once')
      } else {
        if (typeof document !== 'undefined') {
          container = document.createElement('div')
          container.innerHTML = serverOutput
          act(() => {
            ReactDOM.hydrate(testHarness(renderProps), container!)
          })
        }
      }
    },

Option 3: pass in document

This is the approach I usually take, but it would be a breaking change and most people don't like passing in globals this way.

hydrate(_document: Document) {
// ...
}

// Then to call it 
hydrate(document)

[mpeyper]

As you suspected, the hydrate function is only intended to be called in a client environment with a document available while render should be callable in a purely node environment without a document.

As such, I don’t see option 1 so much as a Band-Aid as ensuring the test environment matches the scenario you are wanting to test.

That said, I think there are improvements we can make to the hydrate function to improve the error received in this situation. I’m on my phone so I won’t attempt a code block, but I could envision an else block on that if statement in option 2 that throws an error explaining that a global document is required before you can call hydrate rather than just silently doing nothing. It might be worthwhile extending that nicety to the DOM renderer’s render function.

As for option 3, I’m not opposed to the idea of being able to pass in a document to render into, which could be defaulted to the global document for backwards compatibility. I would think it needs to be supported in the DOM renderer as well for consistency though.

Speaking of consistency, another option might be to follow what react-testing-library does and allow the user to pass in a container to render into and bypasses creating one from the document all together (docs).

[jsjoeio]

As such, I don’t see option 1 so much as a Band-Aid as ensuring the test environment matches the scenario you are wanting to test.

Ah, happy to hear you say that. I think I confused myself around the original PR and what it was fixing. I think I can add a test that ensures renderHook does work in a SSR environment. I'll see if I can get a PR in soon.

That said, I think there are improvements we can make to the hydrate function to improve the error received in this situation. I’m on my phone so I won’t attempt a code block, but I could envision an else block on that if statement in option 2 that throws an error explaining that a global document is required before you can call hydrate rather than just silently doing nothing. It might be worthwhile extending that nicety to the DOM renderer’s render function.

That's a great point! Should I open a separate issue for that, add details and then you can mark as a "good first issue"? (I can comment on it saying I'm happy to mentor whoever picks it up).

[mpeyper]

I don’t mind if you want to use this issue for it, or raise a new one if that makes it more consumable to new comers, whichever you prefer.

[jsjoeio]

I don’t mind if you want to use this issue for it

Guess that's easier. I'll update the title and add some notes here.

[jsjoeio]

Problem

hydrate() is assumed to be called in a client environment (it uses document). However, if you do so in a non-client environment, you get ReferenceError: document is not defined.

Solution

Check for document in hydrate call and throw user-friendly error.

Additional Notes

I know how to solve this but I'd rather let someone else do it and offer mentorship in return. If someone else wants to take this on and wants guidance, ping me.

@mpeyper can you add the "good first issue" label?

This is a great resource: Assertion Functions in TypeScript

[amovar18]

@mpeyper can I pick this up? If so, the user friendly error message can be Hydrate function can only be called in a client environment with a document available. ?