-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
#2168: Added unit tests to check proper component unmounting #2195
#2168: Added unit tests to check proper component unmounting #2195
Conversation
test/browser/render.test.js
Outdated
} | ||
} | ||
|
||
// The only difference to the previous test is the extra div here |
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.
Yes but this <div>
is not part of your vnode tree since you start underneath and then target it with newScratch.firstElementChild
did this work with Preact 8? This isn't supported in Preact X atleast since this falls outside your render root.
<scratch>
<div> <-- you start replacing node here
<div id="child"> <-- your root starts here
</scratch>
^ this is intended
I don't mind merging in the first test, the second has been discussed on your issue right? Do you think we could do more to help out in this case? |
Yes, the issue has been discussed and there is a reasonable workaround for now. Keeping the first test would still be great like you said. Thank's for the support! |
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.
Thanks for this, more tests prevents us from accidentally shipping regressions!
🚀 This PR has been merged! Once a new release is created, any changes will become available on npm. Until then, you can load and install it directly from the Pika CDN:
|
…reactjs#2195) * preactjs#2168: Added unit tests to check proper component unmounting * preactjs#2168: Remove test onlies * remove the second test (invalid) Co-authored-by: Jovi De Croock <decroockjovi@gmail.com>
I added two tests that show the bug: #2168