Errors handing for effect callbacks

[mpeyper]

Originally raised in #305, errors throw in useEffect (and useLayoutEffect) callbacks are not being captured in result.error like other rendering and async errors.

A quick test showed that using an error boundary would catch the errors in the effects. We used to have a an error boundary for error handling, but after issues found in #50 and #74, it was removed.

I'm proposing we reintroduce the error back into the mix.

As an alternative approach, I think it might also worth exploring an alternative where we no longer try to catch errors and just have renderHook, rerender and all the async utils just throw them. This would be a breaking change, but I suspect it will actually be a more ergonomic and idiomatic testing experience. I haven't put too much thought into this approach yet, so there might be some issues with it.

[alex-cory]

I'm also having issues with this. Here's what's going on with me.

const useTest = () => {
  const makeTest = method => async () => {
    throw Error(method)
  }

  const test = { get: makeTest('get') }

  useEffect((): any => {
    const runGet = async () => {
      try {
        await test.get()
      } catch (err) {
        // This console.log will work
        console.log('(useEffect) error', err)
        // But if we throw it to try and bubble it up, it will throw the error, but it
        // won't get caught in the ErrorBoundary as seen in the test below
        throw err
      }
    }
    runGet()
  }, [])
  return test
}

And the test

it('should throw an error', async (): Promise<void> => {
    let caughtError = null
    class ErrorBoundary extends React.Component {
      state = { hasError: false }
      componentDidCatch(error: any) {
        this.setState({ hasError: true })
        // this however, is not run. Or at least it doesn't output to the console
        console.log('Did this run?')
        caughtError = error
      }
      render = () => !this.state.hasError && this.props.children
    }
  
    const wrapper = ({ children }: { children?: ReactNode }): ReactElement => <ErrorBoundary>{children}</ErrorBoundary>
  
    const { result, waitForNextUpdate } = renderHook(useTest, { wrapper })

    // regardless of whether this is here, still not getting the `error`
    await waitForNextUpdate()

    // and neither of these have the correct values
    console.log('result.error', result.error)
    console.log('caughtError', caughtError)
}

It would be nice if there was a codesandbox or something similar to setup a runnable example for you.

Not sure if this is fully relevant, but. facebook/react#14326 (comment)

[mpeyper]

I've been working on this recently but it has me stumped.

I can use an Error Boundary, but once it triggers, I'm finding it very difficult to reset it so that a rerender call gets another chance to run the hook.

I've also discovered that the act call around the react-test-render create function will also throw the error, so I tried moving the current try/catch error handling out of the TestHook component and around the the render call itself, which worked ok for the the basic examples, but is proving quite difficult to also support the async use cases (where a subsequent render causes the error to be thrown.

I'm also just generally struggling to suppress the error output from React itself the error escapes the TestHook render function, so all-in-all, this issue is a huge pain.

I've added a few tests that cover the existing failure case. They are ignored for now, but can be enabled by removing the .skip from this line. If anyone wants to take a run at this, I would be very grateful as it's doing my head in right now.

[goce-cz]

I'm very much in favor of just throwing the error from the renderHook and from anywhere else. But maybe it's just me who doesn't see the benefit of catching it.

If I want to prove that the hook throws, I can simply simply do:

expect(() => renderHook(...)).toThrow(...)

Right? The same applies to rerender of an already mounted hook.

I learned just recently that the error is caught and I'm super annoyed by the fact that I will have to add:

expect(result.error).toBeUndefined()

to basically every existing (and future) test to avoid false positive results. 😢

Probably I should run this assertion after every rerender and act as well, shouldn't I? 😮 This makes the tests heavily bloated.

If I'm doing something wrong, please advise as I'm a bit desperate at the moment.

[mpeyper]

Hi @goce-cz,

There will only ever be one of result.current and result.error set at any point in time, so if you are asserting that result.current has a specific value then there is no need to assert that result.error is undefined. Similarly, result.current is actually a getter that will throw the error in result.error if there is one there, so the error you see in your test failure will be the one preventing your expected value from being there (rather than a generic, "unexpected value: undefined" message.

Where things get a bit more difficult is when there is no return value from your hook and you are relying solely on a mock verification or something similar to test that something happened. In general, I would say that if your expectation was met, then adding an assertion the result.error is undefined is not going to be adding a lot of value to your test (it's not zero, but it's not much).

The reason it's caught and exposed this way is for updates to the component that happen asynchronously. Imagine a hook like this:

function useValue(initialValue = 0) {
  const [value, setValue] = useState(initialValue)

  if (value < 0) {
    throw Error('value must positive')
  }

  const updateAsync = (newValue) => setTimeout(() => setValue(newValue), 100)

  return { value, updateAsync }

And the test:

test('should update value asynchronously', async () => {
  const { result, waitForNextUpdate } = renderHook(() => useValue()
  
  act(() => {
    result.current.updateAsync(-5)
  })

  await waitForNextUpdate()

  expect(result.current.value).toBe(-5)
})

The error here does not occur during the renderHook or act calls nor is the rerender explicitly called, so when should the error be raised. It's probably worth noting that the current functionality was written before we had the async utils as I think these days it would be reasonable to say that if the waitForNextUpdate rejected in this case it would be sufficient, but there are possibly other cases (like perhaps the more general waitFor?) where that error would be lost.

[github-actions]

🎉 This issue has been resolved in version 5.0.0-beta.12 🎉

The release is available on:

• npm package (@beta dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[github-actions]

🎉 This issue has been resolved in version 5.0.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀