Disconnect MutationObserver synchronously in wait-for #801
Conversation
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit a8dffff:
|
.huskyrc.js
Outdated
| @@ -1 +0,0 @@ | |||
| module.exports = require('kcd-scripts/husky') | |||
There was a problem hiding this comment.
@kentcdodds I removed this to be able to commit. I had an infinite loader that you can see in this little video. (I am using ubuntu 18)
Did you ever heard of any problem of this kind ?
There was a problem hiding this comment.
git commit --no-verify avoids any pre-commit hooks. Any change to the pre-commit hook should be dealt with in a separate issue/pr.
There was a problem hiding this comment.
Oh thanks for the command, I totally forgot it exists ^^
I push directly the revert :)
(by the way I didn't expect to merge this change on master, it was only to be able to commit hoping someone will help me like you did ;) )
Otherwise do you have any idea why could the test run infinitely ? (in the hook)
There was a problem hiding this comment.
No idea. I have husky disabled globally since it doesn't work well with my workflow (commit often, commit failing tests etc).
There was a problem hiding this comment.
Ahaha no problem :)
|
The CI failed because of the new node v15, it is waiting for a response after the |
src/wait-for.js
Outdated
| // Call also the disconnect method on setImmediate for projects not using the MutationObersver implementation of jsdom. | ||
| // But using for example `@sheerun/mutationobserver-shim` which provokes infinite loop when being called immediately on `onDone` function |
There was a problem hiding this comment.
This looks fine to me. I'm only concerned about the implications of calling disconnect twice. I guess it's fine, but I'm not sure it's worthwhile having code that is only around to support a shim that nobody should be using (because they should be using a modern version of jsdom or a real browser environment). Anyone want to argue against just removing the next line?
There was a problem hiding this comment.
I have commented and on the issue and proposing two solutions:
- remove the line as you suggest Test really slow because of MutationObserver not disconnecting directly #800 (comment)
- check if we are using MutationObserver of
jsdomor another custom impl Test really slow because of MutationObserver not disconnecting directly #800 (comment)
There was a problem hiding this comment.
If nobody should use shim, it's good for me to remove it.
Thank you Kent and Marco, I'm gonna remove it asap
Does someone know what I should do for the breaking CI ? (if there is something I can do)
|
I'm fixing the build failure now: #802 |
|
I'm not sure why, but travis seems to be taking forever to run our builds. I'll look at this again tomorrow. |
Codecov Report
@@ Coverage Diff @@
## master #801 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 26 26
Lines 719 719
Branches 184 184
=========================================
Hits 719 719
Continue to review full report at Codecov.
|
|
@all-contributors please add @romain-trotard for code |
|
I've put up a pull request to add @romain-trotard! 🎉 |
|
🎉 This PR is included in version 7.26.4 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
Here is a little PR after opening this issue #800
(my investigation is on it)
What:
My test are slow because of MutationObersion disconnections made at the end of my tests.
Why:
Currently
observer.disconnect()is called in setImmediate, I guess the main thread is not available until the end of the test, so the check callback is called too much times.This was done in this PR #102
How:
All my explanations are visible here: #800 (comment)
After making many test, I didn't see any problem to call both:
Checklist:
docs site N/A