Skip to content

wait and waitForValueToChange async utils #200

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

Merged
merged 6 commits into from
Nov 5, 2019
Merged

wait and waitForValueToChange async utils #200

merged 6 commits into from
Nov 5, 2019

Conversation

mpeyper
Copy link
Member

@mpeyper mpeyper commented Oct 14, 2019

What:

Added additional async utils:

  • wait(callback)
  • waitForValueToChange(selector)

Resolves #173

Why:

These additional utils privide similar to the wait and waitForElement/waitForElementToBeRemoved from the dom-testing-library async utils.

This compliments the waitForNextUpdate utility we already provide that shares similarities to the waitForDomChange utility.

How:

Theses utilities are layers on top of waitForNextUpdate, running their relevant checks on each rerender of the test component.

I chose to be constsent with dom-testing-library by not pushing any values into the callbacks, but rather allow/expect the existing APIs to be used within them, this means testing for a value to change looks like:

test('should wait for value to change', async () => {
  const { result, waitForValueToChange } = renderHook(() => useSomeHook())

  await waitForValueToChange(() => result.current.someValue === 'expected value')

  // perform assertions
}

instead of

test('should wait for value to change', async () => {
  const { waitForValueToChange } = renderHook(() => useSomeHook())

  await waitForValueToChange((currentResult) => currentResult.someValue === 'expected value')

  // perform assertions
}

or something similar.

I anticipate this to cause some confusion when people first start using it so I have tried to call it out explicitly in the API reference docs. I have not made any changes to the usage docs yet as I want to get a feeling on what people are struggling with and how best to describe the intent and usage before making those changes.

I'm not super sold on the name of waitForValueToChange and may change it, so if you have any suggestions, I'd love to hear them.

I'm also thinking about refactoring the implementation to have wait as the underlying utility instead of waitForNextUpdate by proving a () => {} callback to wait. I think this might make more conceptual sense to anyone else coming in and trying to understand how these all fir together.

Checklist:

  • Documentation updated
  • Tests
  • Ready to be merged

@codecov
Copy link

codecov bot commented Oct 14, 2019

Codecov Report

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

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #200   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           3      4    +1     
  Lines          56     95   +39     
  Branches        5     15   +10     
=====================================
+ Hits           56     95   +39
Impacted Files Coverage Δ
src/pure.js 100% <ø> (ø) ⬆️
src/asyncUtils.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 6cb2832...66a49f7. Read the comment docs.

@mpeyper
Copy link
Member Author

mpeyper commented Oct 15, 2019

I'm also thinking about refactoring the implementation to have wait as the underlying utility

I had a stab at this and it was actually way more complicated than I expected to flip the waitForNextUpdate and wait relationship around due to how when the render listeners resolve. This could probably be overcome with an overhaul of that system, but I chose not to do that at this time.

I did manage to refactor waitForValueToChange to use wait under the hood indead of duplicating much of the logic. This introduced a new option into the mix, suppressErrors, which is true by default for wait but false by default for waitForValueToChange. I'm considering not surfacing this as an API option as I feel like it's starting to diverge a bit from the intended goal of providing dom-testing-library inspired utilites, which basically offer a timeout and an interval (which doesn't apply to us as we use the next render as the trigger to retest the conditions) option, but I'm not sure yet.

I've also been thinking about the name waitForValueToChange and I'm wondering if waitForNextValue is perhaps a bit better? It's consistent with the waitForNextUpdate naming and a bit more concise to type, but I'm not sure it's as descriptive about what it's waiting for. I dunno. Happy to hear thoughts on this.

@mpeyper
Copy link
Member Author

mpeyper commented Nov 5, 2019

I've dragged my heels on this long enough on this. I've decided to take the names as they are now and we can always change them if better suggestions come along or if people don't understand them.

@mpeyper mpeyper merged commit 38124fa into master Nov 5, 2019
@mpeyper mpeyper deleted the pr/async-utils branch June 8, 2020 13:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Warning about using different versions of act() when wait()ing on an async effect
1 participant