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

fix: flush pending hooks effects in rerender #32

Merged
merged 2 commits into from
May 19, 2022

Conversation

aduth
Copy link
Contributor

@aduth aduth commented Sep 20, 2020

What:

Call preact/test-utils act which manages the same setupRerender behavior implemented previously in preact-testing-library, but also flushes any pending hook effects.

Why:

I had encountered an issue in one of my own projects using @testing-library/preact-hooks where calling hook's setState would not trigger a render, in a case where the state setter may be called asynchronously.

How:

This follows and defers to the internal implementation of preact/test-utils's act function, which also calls setupRerender's resulting rerender function, but only after handling flushing behavior. The changes here effectively incorporate that behavior.

See also: preactjs/preact#1437

Checklist:

  • Documentation added (N/A)
  • Tests
  • Typescript definitions updated (N/A)
  • Ready to be merged

Call preact/test-utils act which manages the same setupRerender behavior implemented previously in preact-testing-library, but also flushes any pending hook effects
@ombene
Copy link

ombene commented May 13, 2021

I'm experiencing this issue as well, and likewise this fix seems to work well for me. Thanks @aduth! Maintainers: any update on this?

@mbradley36
Copy link

Is there any update on this? I am running in to this issue as well.

@parkjh
Copy link

parkjh commented Mar 3, 2022

@nickmccurdy Hello, I just wanted to ask if this pr is planned to be merged / released any time soon?

@bmarti44
Copy link

bmarti44 commented May 6, 2022

we're running into this issue as well - is there any additional verification we can do to help push this through?

@oteoe oteoe mentioned this pull request May 9, 2022
@kentcdodds
Copy link
Member

There are no active maintainers on this project. If anyone's willing to step up as an official committed maintainer and can present some trustworthy credentials (good OSS behavior in the past) then I'd be willing to give maintainer rights. Until then, I'm afraid there's just no bandwidth for maintaining this project.

@JoviDeCroock
Copy link
Member

Hey @kentcdodds I don't mind taking over some of the maintenance on this project if you don't mind

@kentcdodds
Copy link
Member

Thanks @JoviDeCroock! I'll get you added to the team right away.

@JoviDeCroock
Copy link
Member

JoviDeCroock commented May 18, 2022

@kentcdodds is there something wrong or does the CI need explicit approval in travis? We could bypass this by enabling #44 but that would require access to protected branches as the travis check is mandated

@kentcdodds
Copy link
Member

Eek, I didn't realize this was on Travis. I'm afraid I don't think I have access to the travis stuff for this project. It may be that we first need to update the CI pipeline... I'm going to remove the required TravisCI check for now.

@kentcdodds
Copy link
Member

I should add @JoviDeCroock that you don't need my permission or anything for this project at all. If you want to change the setup or whatever that's totally up to you. Like I said, I don't have the bandwidth to work on this project at the moment so feel free to take complete ownership over it. I appreciate you giving it some of your time and attention!

@JoviDeCroock
Copy link
Member

@kentcdodds sorry to bother you but one last request could someone do a manual request of the git tag for 3.0.0 as we dropped node 10 support, however the semantic release was bugged. Fixed that in #54

@kentcdodds
Copy link
Member

Happy to help. I don't understand what you're asking me to do though. Could you elaborate?

@JoviDeCroock
Copy link
Member

This run created a tag on github but the publish failed https://github.com/testing-library/preact-testing-library/runs/6494824522?check_suite_focus=true - the fix however happens in a later commit so I can't just rerun it. This is a manual publish I assume

@kentcdodds
Copy link
Member

Dang it. I always hate when semantic-release falls off the rails. I never know how to get it back going again. I'll look into it.

@kentcdodds
Copy link
Member

Based on the error message from the latest build, it's not a manual publish we need:

The current error message when publishing

npm notice Beginning October 4, 2021, all connections to the npm registry - including for package installation - must use TLS 1.2 or higher. You are currently using plaintext http to connect. Please visit the GitHub blog for more information: https://github.blog/2021-08-23-npm-registry-deprecating-tls-1-0-tls-1-1/

@JoviDeCroock
Copy link
Member

I fixed that error in #54

@kentcdodds
Copy link
Member

Ok, so you're saying that if I do a manual publish of the package this should fix itself? Do you know what version I need to publish? (Sorry, I just don't have too much time to look into it).

@JoviDeCroock
Copy link
Member

JoviDeCroock commented May 18, 2022

First and foremost sorry for the spam on your PR 😅 this tag should be ready to publish so going to that tag and running the build and then publish with the version set to 3.0.0

@kentcdodds
Copy link
Member

Actually, you should have publish rights anyway, so I added you to the npm org 👍

@plantvsbirds
Copy link

Let's goooo!!!

@JoviDeCroock JoviDeCroock merged commit 3318e0d into testing-library:master May 19, 2022
@github-actions
Copy link

🎉 This PR is included in version 3.0.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@aduth
Copy link
Contributor Author

aduth commented May 19, 2022

Glad to see this shipped! Thank you @kentcdodds and @JoviDeCroock for helping to keep this project maintained 🙂

@aduth aduth deleted the fix-flush-hooks branch May 19, 2022 23:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants