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

feat(cleanup): remove unnecessary flush microtasks as already handled by act #511

Merged
merged 2 commits into from
Dec 8, 2020

Conversation

mpeyper
Copy link
Member

@mpeyper mpeyper commented Dec 7, 2020

What:

Remove some particularly complicated code that flushes microtasks as part of cleaning up the rendered hooks.

Why:

I noticed this change in RTL from which we originally borrowed the flush-microtasks.js implementation from. The interesting part is that we already have our cleanup wrapped in act so the code was even less required.

How:

I deleted the file and removed the import and usage of it.

One difference to the RTL PR to note is that they also change cleanup from an async function to a regular function. I chose not to do this as we recently introduced the addCleanup functionality in v3.5.0 which allowed user provided cleanup to be async and I didn't want to restrict them to being sync only, plus it avoided a breaking change.

Checklist:

  • Tests
  • Ready to be merged

I wonder if this should go out into a beta release like they did in RTL or if their beta is proof that it's fine and we just release the minor?

@codecov
Copy link

codecov bot commented Dec 7, 2020

Codecov Report

Merging #511 (c365fbb) into master (c7ee0b4) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #511   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            4         4           
  Lines          128       127    -1     
  Branches        24        24           
=========================================
- Hits           128       127    -1     
Impacted Files Coverage Δ
src/cleanup.js 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 c7ee0b4...c365fbb. Read the comment docs.

@mpeyper mpeyper added the request for comment Discussion about changes to the library label Dec 8, 2020
@joshuaellis
Copy link
Member

When did they release this beta? Were there any issues open against it?

@mpeyper
Copy link
Member Author

mpeyper commented Dec 8, 2020

Beta was released on 30/8 and full release went out on 2/9.

I don't know if there were any issues raised, but there were no additional commits between bets and release.

@joshuaellis
Copy link
Member

@mpeyper Can't see any issues relating to that release open or closed & the comments in the PR seem really positive. IMO we could just release this.

@mpeyper
Copy link
Member Author

mpeyper commented Dec 8, 2020

ok, here we go...

@mpeyper mpeyper merged commit c53b56b into master Dec 8, 2020
@github-actions
Copy link

github-actions bot commented Dec 8, 2020

🎉 This PR is included in version 3.7.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
Labels
released request for comment Discussion about changes to the library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants