Formed in 2009, the Archive Team (not to be confused with the archive.org Archive-It Team) is a rogue archivist collective dedicated to saving copies of rapidly dying or deleted websites for the sake of history and digital heritage. The group is 100% composed of volunteers and interested parties, and has expanded into a large amount of related projects for saving online and digital history.
Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upDisconnect 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:
|
| @@ -1 +0,0 @@ | |||
| module.exports = require('kcd-scripts/husky') | |||
romain-trotard
Nov 1, 2020
Author
Contributor
@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 ?
@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 ?
eps1lon
Nov 1, 2020
Member
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.
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.
romain-trotard
Nov 2, 2020
Author
Contributor
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)
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)
eps1lon
Nov 2, 2020
Member
No idea. I have husky disabled globally since it doesn't work well with my workflow (commit often, commit failing tests etc).
No idea. I have husky disabled globally since it doesn't work well with my workflow (commit often, commit failing tests etc).
romain-trotard
Nov 2, 2020
Author
Contributor
Ahaha no problem :)
Ahaha no problem :)
|
The CI failed because of the new node v15, it is waiting for a response after the |
| // 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 |
kentcdodds
Nov 2, 2020
Member
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?
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?
marcosvega91
Nov 2, 2020
•
Member
I have commented and on the issue and proposing two solutions:
- remove the line as you suggest #800 (comment)
- check if we are using MutationObserver of
jsdom or another custom impl #800 (comment)
I have commented and on the issue and proposing two solutions:
- remove the line as you suggest #800 (comment)
- check if we are using MutationObserver of
jsdomor another custom impl #800 (comment)
romain-trotard
Nov 2, 2020
•
Author
Contributor
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)
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! |
|
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