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

wait not triggered on non hook updates #393

Closed
AugustinLF opened this issue Jun 17, 2020 · 2 comments · Fixed by #408
Closed

wait not triggered on non hook updates #393

AugustinLF opened this issue Jun 17, 2020 · 2 comments · Fixed by #408
Labels
enhancement New feature or request

Comments

@AugustinLF
Copy link

Describe the feature you'd like:

wait from asyncUtilities is only called when the hooks updates, and can't be used to check on side-effects. As far as I know, this is different from the behaviour of the wait exposed from @testing-library/dom or the one of @testing-library/react-native which to me was very confusing.

I have a hook that does some async work, then dispatches a redux action. I was doing a await wait(() => expect(store.dispatch).toHaveBeenCalled()). My test never resolved, replacing the wait from renderHook to the one exported in @testing-library/react made my test pass.

Suggested implementation:

I didn't look enough to the implementation of these two libraries' wait, but perhaps something similar could be made? I would be totally ok with contributing and opening such a PR if you're ok.

Teachability, Documentation, Adoption, Migration Strategy:

There shouldn't be any change of API, and this would not be a breaking change, so I guess this wouldn't be ok?

@AugustinLF AugustinLF added the enhancement New feature or request label Jun 17, 2020
@mpeyper
Copy link
Member

mpeyper commented Jun 17, 2020

Hi @AugustinLF,

You're correct, the implementation between the two is different. This is the second issue (this was the first) now where that decision seems to be causing confusion and/or problems. This issue feels like a more compelling argument for making the change than previously, so I'm happy to explore the idea in more detail.

Essentially, all the other async utils are build on top of waitForNextUpdate. I've thought for a while that perhaps this should be renamed to waitForNextRender and a new utility could be introduced using the waitForNextUpdate name that checks is result.current has actually changed (possible a shallow compare?) before resolving the promise.

wait should be renamed waitFor to match the latest dom-testing-library naming conventions and waitForValueToChange does not have an exact equivalent in the dom-testing-library utilities, so it's name is probably fine.

The checks for waitForNextRender should probably not move to an interval and triggered when the hook renders still makes more sense to me here. Similarly for waitForNextUpdate.

The intended use case for waitForValueToChange was always for selecting a value from result.current, but there has never been any formal limitation on waiting on other values (other than the render being required to actually run the check), so this one may be a candidate for moving to an interval check.

I think it also makes sense for waitForNextUpdate to trigger on the renders instead of an interval, although in some ways, waitForNextUpdate actually makes a lot of sense to be built on top of waitForValueToChange but passing in () => result.current as the callback.

My biggest concern is if two renders occur between intervals that change the value and change it back again, that the change will be missed and the test will time out. I'm not sure how possible it would be to trigger the check on an interval or when the hook renders, essentially giving the best of both worlds?

Finally, waitFor is the one that makes the most sense to run on an interval. Again, the original use case was for asserting the values in result.current, but there's no reason why the callback couldn't use any values. Similarly to the above, I don't think it would hurt if the checks triggered on an interval as well as when the hook rendered, which may remove the change of a missed check from multiple renders.

Happy to hear any thoughts or counter points to any of this. I'm definitely not attached to the existing implementations or any of these specific ideas, so feel free throw it all away and think about it from a fresh slate.

@AugustinLF
Copy link
Author

What you wrote makes a lot of sense, and I don't see any issue with it. waitForUpdate was actually pretty clear to me, its behaviour matching what I would expect.

You're right about the risk of the conflict between intervals and updates, but I feel like that would be an ok deal. If people really care about catching all the different states of the hook, they would probably use waitForNextUpdate, right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
2 participants