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

Refactor async utils #537

Merged
merged 4 commits into from Jan 12, 2021
Merged

Refactor async utils #537

merged 4 commits into from Jan 12, 2021

Conversation

mpeyper
Copy link
Member

@mpeyper mpeyper commented Jan 12, 2021

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 and interval 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 being awaited 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 around waitFor to turn a selector into a callback. In the new version, there is a base wait 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 around wait to add their pieces of nuanced logic.

Because the default behaviour of waitFor was to suppress any errors (so expect could be used in the callback), there was a really clunky option called suppressErrors that waitForValueToChange 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 remove suppressErrors 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 overlapping act 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 internal act logic and the overall complexity of the base wait utility.

Checklist:

  • Documentation updated
  • Tests
  • Ready to be merged

Note: this is a breaking change

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
Copy link

codecov bot commented Jan 12, 2021

Codecov Report

Merging #537 (36f25fb) into beta (bab38d9) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##              beta      #537   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           14        14           
  Lines          199       202    +3     
  Branches        29        27    -2     
=========================================
+ Hits           199       202    +3     
Impacted Files Coverage Δ
src/core/asyncUtils.ts 100.00% <100.00%> (ø)
src/helpers/promises.ts 100.00% <100.00%> (ø)

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 bab38d9...36f25fb. Read the comment docs.

@mpeyper
Copy link
Member Author

mpeyper commented Jan 12, 2021

I wonder if these changes fix the act issues I was having when trying to implement a preact renderer?

Edit: Nope, still an issue

Copy link
Member

@joshuaellis joshuaellis left a 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)

@joshuaellis joshuaellis merged commit 394f65a into beta Jan 12, 2021
@joshuaellis joshuaellis deleted the feature/async-utils-refactor branch January 12, 2021 15:41
@github-actions
Copy link

🎉 This PR is included in version 5.0.0-beta.10 🎉

The release is available on:

Your semantic-release bot 📦🚀

@github-actions
Copy link

🎉 This PR is included in version 5.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants