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

Prevent crash in environments where Element.prototype.getAnimations is not available #3473

Merged
merged 6 commits into from
Sep 11, 2024

Conversation

RobinMalfait
Copy link
Member

@RobinMalfait RobinMalfait commented Sep 11, 2024

Recently we made improvements to the Transition component and internal useTransition hook. We now use the Element.prototype.getAnimations API to know whether or not all transitions are done.

This API has been available in browsers since 2020, however jsdom doesn't have support for this. This results in a lot of failing tests where users rely on jsdom (e.g. inside of Jest or Vitest).

In a perfect world, jsdom is not used because it's not a real browser and there is a lot you need to workaround to even mimic a real browser.

I understand that just switching to real browser tests (using Playwright for example) is not an easy task that can be done easily.

Even our tests still rely on jsdom…

So to make the development experience better, we polyfill the Element.prototype.getAnimations API only in tests (process.env.NODE_ENV === 'test') and show a warning in the console on how to proceed.

The polyfill we ship simply returns an empty array for node.getAnimations(). This means that it will be enough for most tests to pass. The exception is if you are actually relying on transition-duration and transition-delay CSS properties.

The warning you will get looks like this:

Headless UI has polyfilled `Element.prototype.getAnimations` for your tests.
Please install a proper polyfill e.g. `jsdom-testing-mocks`, to silence these warnings.

Example usage:
```js
import { mockAnimationsApi } from 'jsdom-testing-mocks'
mockAnimationsApi()
```

Fixes: #3470
Fixes: #3469
Fixes: #3468

This polyfill does 2 things:

1. Shows a warning with information on how to proceed
2. Results in **no** animations at all. Which means that if you test the
   actual timing of transitions, that this will now fail (until the
   polyfill is installed).
Copy link

vercel bot commented Sep 11, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
headlessui-react ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 11, 2024 3:18pm
headlessui-vue ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 11, 2024 3:18pm

Comment on lines +8 to +9
typeof process !== 'undefined' &&
typeof globalThis !== 'undefined' &&
Copy link
Contributor

Choose a reason for hiding this comment

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

should we also check for typeof process.env !== 'undefined' or make the process.env[] lookup an optional chain, just to prevent a crash in some very weird setups:

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah not a bad idea honestly

@plavski
Copy link

plavski commented Sep 11, 2024

Just a note to say that happy-dom, another very popular framework for tests, also does not support getAnimations: https://github.com/capricorn86/happy-dom/blob/afc3692e8dde4d3769706fb36f7818b6d41197bc/packages/happy-dom/src/nodes/shadow-root/ShadowRoot.ts#L185

So any messages should not be jsdom specific imo

If, for whatever reason, the `process` is `null`, or it is available but
`process.env` is not then this could still crash. Using `?.` should help
us in all these situations.
@RobinMalfait
Copy link
Member Author

@plavski the jsdom-testing-mocks has jsdom in the name but isn't specific to jsdom. It has general purpose mocks. I don't think it's useful to try and detect in which environment you are. jsdom is typically the default, and since the mock package is not specific to jsdom (regardless the name) it should be fine to use 👍

@RobinMalfait
Copy link
Member Author

Updated the wording slightly

@plavski
Copy link

plavski commented Sep 11, 2024

@plavski the jsdom-testing-mocks has jsdom in the name but isn't specific to jsdom. It has general purpose mocks. I don't think it's useful to try and detect in which environment you are. jsdom is typically the default, and since the mock package is not specific to jsdom (regardless the name) it should be fine to use 👍

Cool. It was only the wording feeling overly specific that was the issue for me, and your change has cleared that up. Thanks!

@corneliusroemer
Copy link

Just tested this and it all works as expected. Without explicit polyfill there's a warning. With polyfill as suggested, no warning, see https://github.com/loculus-project/loculus/pull/2766/files for implementation details.

corneliusroemer added a commit to loculus-project/loculus that referenced this pull request Sep 12, 2024
…t not interceptors) (#2766)

* deps(website): update headlessui and add jsdom polyfill

see:
- tailwindlabs/headlessui#3473
- tailwindlabs/headlessui#3469

* Update msw, pinning transitive dep msw interceptors to 0.33.3 for the time being

see:
- mswjs/msw#2273
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants