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: support TransitionEvent init properties #865

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

VinceMalone
Copy link

What:

Add support for the init arguments that can be passed to TransitionEventpropertyName, elapsedTime, and pseudoElement.

fireEvent.transitionEnd(node, {
  propertyName: 'opacity',
  elapsedTime: 100,
  pseudoElement: '',
});

Why:

This should be supported out-of-the-box, but jsdom doesn't currently support the TransitionEvent constructor jsdom/jsdom#1781

Because window.TransitionEvent is undefined, window.Event ends up being used to create the event.

const EventConstructor = window[EventType] || window.Event

Which will, of course, ignore the transitionEventInit object.

How:

When the event is being created, look at the event type, if it's TransitionEvent, manually attach the aforementioned properties from eventInit.

Checklist:

  • Documentation added to the
    docs site
  • Tests
  • Typescript definitions updated
  • Ready to be merged

@codesandbox-ci
Copy link

codesandbox-ci bot commented Dec 29, 2020

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 f83aa35:

Sandbox Source
react-testing-library-examples Configuration

@eps1lon
Copy link
Member

eps1lon commented Dec 29, 2020

Could you provide a repro that we can test in jsdom and a real browser. We need to verify this doesn't pollute actual browser tests and why this fix can't be applied upstream i.e. to jsdom.

@VinceMalone
Copy link
Author

@eps1lon, I'm happy to do that, but can you clarify what exactly you're looking for? 😅 I'm not sure I entirely understand.

Copy link
Member

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

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

This is already working in a browser: https://codesandbox.io/s/jsdom-polyfill-transitionevent-bgfz1

You either want to fix that in jsdom or apply a polyfill locally to your testing envrionment. The above codesandbox contains a test that has a minimal polyfill for TransitionEvent.

@marcosvega91
Copy link
Member

Hi @VinceMalone could you please rebase this branch with master?

@codecov
Copy link

codecov bot commented Jan 4, 2021

Codecov Report

Merging #865 (f83aa35) into master (2830eae) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #865   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           26        26           
  Lines          951       957    +6     
  Branches       291       291           
=========================================
+ Hits           951       957    +6     
Impacted Files Coverage Δ
src/events.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2830eae...f83aa35. Read the comment docs.

@VinceMalone
Copy link
Author

@eps1lon I added a check to see if window.TransitionEvent is not a function before monkey patching the fired event — this should prevent any issues with pre-existing working environments.

@eps1lon
Copy link
Member

eps1lon commented Jan 5, 2021

Yeah, I'm not comfortable with monkey patching incomplete environments. This can become super confusing if other libraries don't do that.

Why does a polyfill like https://codesandbox.io/s/jsdom-polyfill-transitionevent-bgfz1 doesn't work for you?

@VinceMalone
Copy link
Author

The polyfill works, but this change might still be worth it for the sake of others. There's a silent failure when an event type isn't supported — because window.Event is used as a backup. So in this case, it's not obvious why the transitionEventInit properties aren't working, despite fireEvent.transitionEnd() being available to use and jsdom being listed as a "supported" environment. This is what drove me to dig into the source and eventually create this PR.

I definitely get that monkey patching this isn't really ideal, but is it worth doing (like DataTransfer) until jsdom supports window.TransitionEvent?

@eps1lon
Copy link
Member

eps1lon commented Jan 6, 2021

I definitely get that monkey patching this isn't really ideal, but is it worth doing (like DataTransfer) until jsdom supports window.TransitionEvent?

I consider monkey-patching DataTransfer a mistake as well. Others seem to disagree.

fireEvent.transitionEnd silently working is just as problematic as polyfilling the properties silently. It'll just fail at a different point e.g. when assuming that changing a CSS property triggers a transition. This is the reason JSDOM doesn't polyfill the event but rather waits for a full implementation of the spec.

I don't believe we're actually improving the problem. Rather we're just kicking the problem to someone else by diverging even more. Before we had JSDOM vs browsers. Now we have JSDOM vs browsers vs testing-library in JSDOM. By using testing-library you're tied to a special environment that's more and more diverging from existing environments and I don't think this is the responsible thing to do.

So yeah, ultimately we should not fall back to window.Event. It's a slippery slope we're on so I'm going to repeat the same argument on every PR.

So in this case, it's not obvious why the transitionEventInit properties aren't working, despite fireEvent.transitionEnd()

That's a bit debateable because in the end you're getting an Event not TransitionEvent. I understand that this isn't obvsious because you receive it in a transitionend event handler but some hint does exist.

I I just don't have a good feeling about these monkey-patches since we have to maintain a very specific use case ("support a partial implementation of the Transition spec") which leaves us open to "how much should we fix in testing-library?" debates.

I'd be way more comfortable if we had a package like jsdom-event-polyfill that people would explicitly opt-in and where we could explain potential draw-backs.

src/events.js Outdated
// TransitionEvent is not supported in jsdom: https://github.com/jsdom/jsdom/issues/1781
if (
EventType === 'TransitionEvent' &&
window.TransitionEvent !== 'function'
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
window.TransitionEvent !== 'function'
typeof window.TransitionEvent !== 'function'

@VinceMalone
Copy link
Author

@eps1lon those are all really good points; I definitely understand the urge to resist going down this path. It's tough to balance the pros can cons of existing decisions (falling back to window.Event or monkey patching DataTransfer), silent errors, and accommodating a wider pit of success. Until then, I'm happy to get onboard with whatever you decide 👍

@100terres
Copy link

100terres commented Apr 14, 2022

This comment is to help others who might stumble on that issue

If anyone is looking for a polyfill or something, here's how we've managed to make it work with jest in our codebase.

hello-pangea/dnd@5ec5a74#diff-3ab6990a471fa18593a64b7c0de508ff724cd1cd0676d882d475fb40fe624660=

@mikedidomizio
Copy link

For Vitest I took the approach of addEventListener to get it working. I add the property on the capture phase of the event so that when onTransitionEnd fires we have the propertyName. This will probably also work with Jest.

I've created a codesandbox to show it working with tests/fireEvent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs discussion We need to discuss this to come up with a good solution needs investigation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants