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

Getting not wrapped in act warning using renderHook #406

Closed
orpheus opened this issue Jul 14, 2020 · 6 comments
Closed

Getting not wrapped in act warning using renderHook #406

orpheus opened this issue Jul 14, 2020 · 6 comments
Labels
bug Something isn't working

Comments

@orpheus
Copy link

orpheus commented Jul 14, 2020

https://codesandbox.io/s/async-surf-8ytrn?file=/src/hook.test.js

Here's a simplified example of a hook I'm using that makes an api call within the hook and also has some state management. Testing the hook from the outside, I have nothing to wrap in act.

I've tried a few of the solutions https://kentcdodds.com/blog/fix-the-not-wrapped-in-act-warning and https://react-hooks-testing-library.com/reference/api#async-utilities but maybe I missed something.

Also the returned value isn't what I expect (it's still the default false). If I use the wait async utils, what would I wait for?

@orpheus orpheus added the bug Something isn't working label Jul 14, 2020
@mpeyper
Copy link
Member

mpeyper commented Jul 15, 2020

The sandbox link you've provided doesn't have a hook in it:

image

In general, I'd say you want to await waitForNextUpdate() which will act and wait for the hook to rerender with the state update. result.current should then reflect the new returned value from the hook function.

@orpheus
Copy link
Author

orpheus commented Jul 15, 2020

The sandbox link you've provided doesn't have a hook in it:

you're right, must not have saved. try again-should be good

In general, I'd say you want to await waitForNextUpdate() which will act and wait for the hook to rerender with the state update

I tried that in the codesandbox and it worked. But when I try it with the hook I'm working with, the test times out

I'm getting

: Timeout - Async callback was not invoked within the 5000ms timeout specified by jest.setTimeout.
Timeout - Async callback was not invoked within the 5000ms timeout specified by jest.setTimeout.Error: 

when doing:

  it('fetch single image', async () => {
    localStorage.setItem('jwt', 'token')
    const { result, waitForNextUpdate } = renderHook(() => useGetImages({ uploaded: true, id: 'test-image-id' }))
    await waitForNextUpdate()
    const { images } = result.current
    expect(images).toHaveLength(1)
  })

I don't want to copy/paste the whole hook in here, but essentially it's doing the same thing as the example hook in the sandbox. Calls an api to fetch an image and before and after sets a loading state using setState (which is where I first get the act warning).

What else could I do to debug?

@mpeyper
Copy link
Member

mpeyper commented Jul 15, 2020

If it's timing out on await waitForNextUpdate(), its because the hook is not rerendering (the logic for resolving that promise is super dumb - if the hook callback returns a new value or throws and error, it resolves). This either because all the state updates have already occurred (common if you're using fake timers) or the promise inside the hook is not resolving.

Given the test you've show, I'm going to assume you aren't using fake timers and the issue is with the promise. The most common reason I've seen for this is that the call is inside a useEffect hook and there is an error occurring that does not trigger a state update of any kind (we have an open issue about errors in this scenario not being reported correctly, so it just sits in the initial state).

To check this, wrap the whole useEffect callback in a try/catch and call your setState with some value that will definitely be different from something expected (setState(error) works well for this). Also make sure the promise has a .catch handler that does the same if you are not using async/await.

Here is an example of what I mean where an error in useEffect would cause the test to timeout instead of fail nicely, which you can see if you remove the try/catch logic: https://codesandbox.io/s/young-river-pruyu?file=/src/hook.test.js

Also, might be worth just logging out directly before the setState call after the promise is meant to resolve. If that logs and then there's something else going on.

@orpheus
Copy link
Author

orpheus commented Jul 15, 2020

Ok so I've tried and tested a few things. To address your comments first, I did log directly before the setState after the promise resolved and that logged fine. The api call is wrapped in a try catch, but I used jest.fn() to mock the api call so it just runs Promise.resolve('someString') (like in my example sandbox). The log I did verified that the jest mock function returned correctly. I also logged in the catch block and nothing returned.

So then I logged outside of the async function in the body of the useEffect and saw that the effect/hook was on an infinite loop. After commenting out several function calls I see that the hook runs indefinitely after a setState call. In my IDE I see this through logs running over and over, but in codesandbox, the test just freezes and doesn't complete (it won't log). Here's an example of what I mean.

https://codesandbox.io/s/renderhook-setstate-test-q1sz1?file=/src/useGetImages.js

Notice the test never finishes/logs.

@mpeyper
Copy link
Member

mpeyper commented Jul 15, 2020

in your renderHook call, you're passing in [] to useGetImages. Because you're defining it inline with the call it creates a new reference every time the hook callback is run, ie.every render.

Generally this would be fine, but because you're then using imageDto in the dependency array for useEffect, the effect is going to rerun every render.

Again, generally this is fine, but because you then setImages in the effect with the new reference of imageDto it triggers a new render, which gets a new reference, which triggers another render, new reference, render, reference, render.... you get the point.

This can be fixed by changing the test to define the argument outside the renderHook callback:

const imageDTO = [];
const { result } = renderHook(() => useGetImages(imageDTO));
expect(result.current.images).toHaveLength(0);

Another alternative (and this will very much depend on your actual requirements and what the rest of the logic in your real hook actually does), is to use a ref to manage which reference to imageDto to use:

const useGetImages = imageDto => {
  const imagesRef = useRef(imageDto);

  useEffect(() => {
    imagesRef.current = imageDto;
    console.log("ran effect");
  }, [imageDto]);

  console.log("ran hook");
  return { images: imagesRef.current };
};

I'll leave it up to you if that is a reasonable alternative. I'll admit it's a bit more complicated and harder to read and it won't implicitly render if imagesRef.current is updated, so you'd need to have some other state floating around that results from the effect for this method to be effective.

@orpheus
Copy link
Author

orpheus commented Jul 15, 2020

Because you're defining it inline with the call it creates a new reference every time the hook callback is run, ie.every render.

Yup, okay so that takes care of it.

  • Adding await waitForNextUpdate() clears the act warning and
  • Defining the arguments outside of the hook + renderHook prevents the infinite loop, allowing the waitForNextUpdate to complete.

I'm going to close this, thank you very much for your help.

Should have caught the inline variable sooner, but I assumed renderHook wouldn't create it anew for some reason - makes sense that it does though.

@orpheus orpheus closed this as completed Jul 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants