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

feat: upgrade to JSDOM@22 #13825

Merged
merged 4 commits into from
Sep 19, 2023
Merged

feat: upgrade to JSDOM@22 #13825

merged 4 commits into from
Sep 19, 2023

Conversation

SimenB
Copy link
Member

@SimenB SimenB commented Jan 26, 2023

Summary

This is a breaking change. But since I spent the time debugging to understand why it exploded horribly (landed in #13972) I thought to open a PR now.

Test plan

CI

@SimenB SimenB added the Pinned label Jan 26, 2023
@SimenB SimenB added this to the Jest 30 milestone Jan 26, 2023
// Note that this.global.close() will trigger the CustomElement::disconnectedCallback
// Do not reset the document before CustomElement disconnectedCallback function has finished running,
// document should be accessible within disconnectedCallback.
Object.defineProperty(this.global, 'document', {value: null});
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added in #5955 - @mjesun you don't by any chance remember what it was added for? this.global.document is undefined after calling this.global.close();

// Note that this.global.close() will trigger the CustomElement::disconnectedCallback
// Do not reset the document before CustomElement disconnectedCallback function has finished running,
// document should be accessible within disconnectedCallback.
Object.defineProperty(this.global, 'document', {value: null});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this fix #12670? Worth adding a test?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

quite possibly...

Copy link
Contributor

@eps1lon eps1lon Jan 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/eps1lon/react-testing-library-error-repro/tree/upgrade-jsdom now errors with

Error: The `document` global was defined when React was initialized, but is not defined 
anymore. This can happen in a test environment if a component schedules an update from
 an asynchronous callback, but the test has already finished running. To solve this, 
you can either unmount the component at the end of your test (and ensure that any 
asynchronous operations get canceled in `componentWillUnmount`), or you can change the 
test itself to be asynchronous.

when using jest-environment-jsdom. This is certainly a clearer error but still unintended.

Going to do some more digging.

Copy link
Member Author

@SimenB SimenB Jan 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I sent facebook/react#22695 which fixed the check for the correct error, so it should match it with old jsdom as well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @SimenB I started looking at this upgrade PR, because many of the JSDOM@21.1.0 patches are mine and I would like to start using them in our Jest tests ASAP 🙂

I did some research about why the document is being set to null during teardown, and why that doesn't really need to be done, and summarized that in #13972.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

react-testing-library-error-repro now errors with

@eps1lon I think this is not an issue with Jest, but a bug in React 17's act implementation, when the act callback returns a promise. Then, after the promise resolves, React will synchronously flush all updates scheduled before resolving. But that code is buggy: it leaves the act environment before doing that flush, so these updates no longer work with the act queue, but are using the regular scheduler. setImmediate etc. And then they run long after the JSDOM env has been teared down.

React 18 has that fixed: it will flush the updates while still inside the act environment.

In your repro example, upgrading to React 18 in package.json immediately fixes the test for me.

@pleunv
Copy link

pleunv commented Feb 6, 2023

Just a heads up: this will break any tests that rely on mocking window.location.assign, which I don't think is an uncommon approach. Still looking for an alternative solution.

@SimenB
Copy link
Member Author

SimenB commented Feb 6, 2023

Right, something like that was my assumption.

An alternative to mocking assign directly could be a separate module.

export assignLocation(...args) {
  return window.location.assign(...args);
}

Then jest.mock that module instead of the API it uses.

@jsnajdr
Copy link
Contributor

jsnajdr commented Mar 2, 2023

There's another breaking change in JSDOM 21.1.0, and also a bug that will need to be fixed before Jest can start using the latest JSDOM version: addition of pageX and pageY getters to MouseEvent in jsdom/jsdom#3484. These getters calculate pageX and pageY incorrectly. Described in detail in https://github.com/jsdom/jsdom/pull/3484/files#r1123237268

Users of Jest and React Testing Library are currently using a workaround for missing pageX and pageY support described in testing-library/react-testing-library#268 (comment), subclassing MouseEvent and assigning to pageX and pageY in constructor. This no longer works with JSDOM 21.1.0, because pageX and pageY are no longer writable. Folks will be able to just create a plain MouseEvent:

new MouseEvent('mousemove', { clientX: 10, clientY: 10 })

without any magic.

@jsnajdr
Copy link
Contributor

jsnajdr commented Mar 2, 2023

this will break any tests that rely on mocking window.location.assign

This is however not new, the location.assign property has been non-configurable even in JSDOM 20, the version we're upgrading from here. jest.spyOn(window.location, 'assign') will fail on current stable version of Jest, too.

What is new is that the window.document, location and top properties are now non-configurable, too. With JSDOM 20 you could assign your mock version to window.location, but now you no longer can.

This is all unfortunate because while all this behavior certainly conforms to the spec, it makes mocking many window properties impossible. Given that unit testing is likely the number one use case for JSDOM, by a large margin, it should provide some escape hatch for mocking.

@karlnorling
Copy link
Contributor

@jsnajdr Any update on this? Is this only a placeholder PR for testing the update?

@jsnajdr
Copy link
Contributor

jsnajdr commented Apr 17, 2023

We've been waiting for a fix of a JSDOM regression, in MouseEvent, discussed in #13825 (comment). The JSDOM fix was accepted and merged yesterday, in jsdom/jsdom#3514. So, as soon as new version of JSDOM is released (21.1.2 or 21.2.0), this Jest PR will be ready to pick it up and ship.

@uladzimirdev
Copy link

@jsnajdr 21.1.2 is released!

@jsnajdr
Copy link
Contributor

jsnajdr commented May 2, 2023

Hi @SimenB 👋 After updating the package.json and the lockfile to JSDOM 21.1.2, this PR should be mergeable. It's my understanding that a major JSDOM upgrade triggers also a major upgrade of Jest.

One thing that should be in the upgrade guide is the MouseEvent changes described in #13825 (comment): the old subclassing workaround, which no longer works, and can be replaced with a normal instantiation of MouseEvent. I can help with writing the section if I know how and where it's being created.

@netlify
Copy link

netlify bot commented May 2, 2023

Deploy Preview for jestjs ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit a36c5ed
🔍 Latest deploy log https://app.netlify.com/sites/jestjs/deploys/65098260835ea60008a961d4
😎 Deploy Preview https://deploy-preview-13825--jestjs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@jsnajdr
Copy link
Contributor

jsnajdr commented May 2, 2023

Seems that @domenic is going to release also JSDOM 22.0.0 any moment now? If so, better wait for that 🙂

@SimenB
Copy link
Member Author

SimenB commented May 2, 2023

This is blocked until our own next major release regardless

@donaldpipowitch
Copy link

When is the next major release of Jest?

@SimenB
Copy link
Member Author

SimenB commented May 2, 2023

No concrete plans. You can probably use yarn's resolutions (or whatever the equivalent in npm/pnpm is) as there's no code changes in jest, so just forcing a newer version of jsdom should be fine

@frosas
Copy link
Contributor

frosas commented May 2, 2023

In npm, you can add this to your package.json:

  "overrides": {
    "jsdom": "^22"
  }

Don't forget to update jest-environment-jsdom to a recent version too (I was getting a "Cannot read properties of null (reading '_location')" error)

If curious, I just tried it on a test suite with 1322 tests that relies heavily on Testing Library and could see it running ~23% faster. Not bad at all.

@jsnajdr
Copy link
Contributor

jsnajdr commented May 3, 2023

Don't forget to update jest-environment-jsdom to a recent version

Yes you need 29.5.0 because it ships the #13972 fix that removes assignment to the no-longer-writable window.document.

I just tried it on a test suite with 1322 tests that relies heavily on Testing Library and could see it running ~23% faster.

I'm happy to see that other projects also see the Testing Library performance improvements with JSDOM styles caching 🎉

@SimenB SimenB changed the title feat: upgrade to JSDOM@21 feat: upgrade to JSDOM@22 May 4, 2023
@donaldpipowitch
Copy link

Damn, I wish I could test this. Looks like we are blocked by jsdom/jsdom#3492 :(

@uladzimirdev
Copy link

@donaldpipowitch I posted a link to semi-solution I found for my case on the issue you linked, maybe it will work for you as well

@SimenB
Copy link
Member Author

SimenB commented May 10, 2023

Ah, v22 drops node 14 as well. Definitely breaking for us then 😅

@donaldpipowitch
Copy link

donaldpipowitch commented May 11, 2023

I think we see performance improvements in the range of 10%-12% :) Thanks everyone!

@gianluca932
Copy link

gianluca932 commented May 26, 2023

Does anyone knows when it will be merged ?
I am experiencing this issue jsdom/jsdom#3452 that has been solved with jsdom 21.1.5

@SimenB
Copy link
Member Author

SimenB commented May 26, 2023

Whenever Jest 30 is released.

You can use resolutions in the meantime - there are no code changes in Jest from 29.5.0

@SimenB SimenB enabled auto-merge (squash) September 19, 2023 07:57
@SimenB SimenB merged commit 2cfb274 into jestjs:main Sep 19, 2023
4 of 7 checks passed
@SimenB SimenB deleted the upgrade-jsdom branch September 19, 2023 11:13
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 20, 2023
@SimenB
Copy link
Member Author

SimenB commented Oct 30, 2023

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants