-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Update JSDOM and create script to automate it #4539
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 2a07c1c:
|
Build for latest commit 2a07c1c is at https://pr4539.build.csb.dev/s/new. |
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 working on this! My only code critique is that the .js
file is still being committed even though it's not used, and this isn't deleting the old ones easier. It would probably be best to rebase this PR to only add the .min.js
file (so the 5MB git object for .js
isn't written to the repository) and remove the old JSDOM 4 files that aren't in use any more.
Unfortunately my test from #4530 is still failing with the same issue, seems like old JSDOM is still used in this sandbox: https://pr4539.build.csb.dev/s/code-sandbox-bug-with-old-jsdom-version-9gcnp?file=/index.test.js |
I've decided to not remove JSDOM 4 yet, since some service workers need to request the file still. I'm planning to remove it in a month though! For the
Hmm, that is weird. I see the template is |
That's still strange, if it's running tests why is it still using JSDOM (and still 4 not 12)? I could tell if it was using the real DOM because the tests would be passing. Do we need to test this without the static template and disable the test runs separately? |
Just for info I have created a repo with the code that is broken on our side https://pr4539.build.csb.dev/s/cold-currying-p7r3g?file=/src/__tests__/index.js |
FYI: My sandbox was based on this so should reproduce the same issue. The bug in |
fe0d4d4
to
ea2a0e8
Compare
Picking this one up again! Apologies for the time, I lost track of this one. I now updated JSDOM to 16.3.0, and only committed the |
Weird, this returns false:
|
Ah, it turns out this was a bug in |
this test is working now, so on our side we are good too 😄 |
Fixes #4530