-
Notifications
You must be signed in to change notification settings - Fork 232
Refactor async utils #537
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
Refactor async utils #537
Conversation
BREAKING CHANGE: `interval` will now default to 50ms in async utils BREAKING CHANGE: `timeout` will now default to 1000ms in async utils BREAKING CHANGE: `suppressErrors` has been removed from async utils
Codecov Report
@@ Coverage Diff @@
## beta #537 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 14 14
Lines 199 202 +3
Branches 29 27 -2
=========================================
+ Hits 199 202 +3
Continue to review full report at Codecov.
|
I wonder if these changes fix the Edit: Nope, still an issue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All looks good, that might fix some of the issues i've been having trying to help someone discord with some odd hooks (that they've written)
🎉 This PR is included in version 5.0.0-beta.10 🎉 The release is available on: Your semantic-release bot 📦🚀 |
🎉 This PR is included in version 5.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
What:
Completely refactor the async utils to be more correct and have more sensible defaults.
Why:
This is something I've wanted to do for a while now.
timeout
andinterval
were both added after the async utils were originally developed, so fo backwards compatibility, we left them as defaulting to off. The catalyst for this change was to add in defaults and require people to opt out of the functionality if they wanted to.I thought it was going to be straight forward to add the default, but when I did I found lots (most) of our async tests began failing. I discovered that we actually had a few bugs in the implementation, particularly around overlapping
act
calls and seemingly a leaking promise that wasn't beingawaited
in some circumstances. So I decided to build it from scratch and see how that went.How:
The main change of the refactor is restructuring thee order of the utils. Previously,
waitForNextUpdate
was the base utility that all others were build off.waitFor
piggy backed off of the render updates and added a layer of interval checking over the top.waitForValueToChange
was a thing wrapper aroundwaitFor
to turn a selector into a callback. In the new version, there is a basewait
util that is not exported, but provides the render updates, interval checking and timeouts, but with minimal logic around what to do other than call the callback. All of the other utilities now provide a thin wrapper aroundwait
to add their pieces of nuanced logic.Because the default behaviour of
waitFor
was to suppress any errors (soexpect
could be used in the callback), there was a really clunky option calledsuppressErrors
thatwaitForValueToChange
would override to throw the errors if a bad selector was used. It was annoying to document, confusing when to set it and I don't believe anyone should ever want to use anything but the defaults. A consequence of this refactor was I was able to removesuppressErrors
without losing the functionality.The other change is that you can no longer
await
multiple utilities at the same time without getting a warning about having overlappingact
calls. I'm not sure why I felt it was so important to support this use case before I can't see any reason why anyone would need to do this. removing it simplified the internalact
logic and the overall complexity of the basewait
utility.Checklist:
Note: this is a breaking change