Add waitForNextUpdate promise for testing async updates

[mpeyper]

What:

Provide ability to wait for hook to rerender to assist in testing asynchronous hooks (e.g. react-async's useFetch)

Why:

While asynchronous behaviour was not explicitly blocked by the library, testing it was often difficult when the internals of a hook used async patterns, but the underlying details (e.g. callback, Promise, etc.) were not exposed out of the hook's interface.

How:

The renderHook function now returns a nextUpdate function that returns a Promise that resolves when the hook is next rendered. The signature and terminology is designed to work with the async/await pattern, e.g.

test('something', async () => {
  const { result, nextUpdate } = renderHook(() => useFetch('/api'))

  expect(result.current.isLoading).toBe(true)

  await nextUpdate()

  expect(result.current.isLoading).toBe(false)
  expect(result.current.data).toEqual({/* ... */})
})

Checklist:

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

Before merging, I'd like some feedback on the interface, terminology and implementation of this feature. Any feedback is welcome.

Resolves #10

[codecov-io]

Codecov Report

Merging #11 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master    #11   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           1      1           
  Lines          13     18    +5     
=====================================
+ Hits           13     18    +5

Impacted Files	Coverage Δ
src/index.js	100% <100%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e4853f3...6e627c8. Read the comment docs.

[mpeyper]

Whoops! these must have snuck into master when I was updating the README a few days ago. Nonetheless, they are not required.

[mpeyper]

not sure if Promise<void> is the most correct type here?

[ab18556]

If we pass the value of the hook's result to nextUpdate/nextValue. I think that the signature should be readonly nextValue: () => Promise<R>

[ab18556]

If we pass the value of the hook's result to nextUpdate/nextValue. I think that the signature should be readonly nextValue: () => Promise<R>

[ab18556]

What if we pass res to resolve? Like this resolvers.splice(0, resolvers.length).forEach((resolve) => resolve(res))
Would it allow us to rename nextUpdate to nextValue and do assertions directly on it?
Like expect(async nextValue()).toEqual('Bob')

[mpeyper]

I did start out with that, but decided that, as there is already a way of accessing the value, it might be confusing to use different variables before and after the await fold

test('something', async () => {
  const { result, nextValue } = renderHook(() => useSomeHook())

  expect(result.current).toBe({/* ... */})

  expect(await nextValue()).toBe({/* ... */})
})

It also falls down when you want to assert a nexted value

test('something', async () => {
  const { result, nextValue } = renderHook(() => useSomeHook())

  expect(result.current.someProp).toBe({/* ... */})

  // doesn't work
  // expect(await nextValue().someProp).toBe({/* ... */})

  const next = await nextValue()
  expect(next.someProp).toBe({/* ... */})
})

or multiple assertions

test('something', async () => {
  const { result, nextValue } = renderHook(() => useSomeHook())

  expect(result.current).not.toContain({/* ... */})
  expect(result.current).toHaveLength(2)

  // fine
  // expect(await nextValue()).toContain({/* ... */})
  // hangs until the test fails
  // expect(await nextValue()).toHaveLength(3)

  const next = await nextValue()
  expect(next).toContain({/* ... */})
  expect(next).toHaveLength(3)
})

It also raises questions about whether it should fire if the result.current is set to the same value as it was already set to. Consider a hook that has some internal state that updated asyncronously, triggering a side effect , but the hook returns a static result or no result at all itself (e.g. a useRedux hook that returns the static store, but a dispatch triggers some internal state to be updated). If it didnt trigger on every render, this would once again be harder to test.

If we wanted to go this way, I think result.nextValue() feels better to me for what it represents, but right now I still feel as the nextUpdate approach in this PR is more consistent, cleaner in more (common, in my opinion) use cases and requires fewer additional variables in complex tests.

[ab18556]

I do agree with you. Anyway, we can access the current result right after we awaited so it doesn't really matter.

About the method's name, I am wondering if it wouldn't be better to have something like the async methods in react-testing-library.

I am talking about the wait, waitForElement and waitForDomChange methods.

Maybe a name like waitForNextUpdate or waitForNextResult would provide a better description?

[mpeyper]

waitForNextUpdate

I like that. waitForNextResult is fine but there are legitimate cases where a custom hook may not have a result at all (i.e. no return value), but waitForNextUpdate covers both nicely.

@ab18556 if you want to submit a PR adding yourself as a contributor (review, ideas, others?), I'll happily merge it.

[ab18556]

I don't feel i did much, but since it was my first contribution to a public project, I am happy to do it.

[mpeyper]

You had more input on this PR than anyone else, and as a library author and maintainer, having anyone actually challenging and validating your work is invaluable and a fantastic contribution you can make (second only to updating the documentation 😉).