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

Use and expose a local copy of fireEvent #75

Merged
merged 5 commits into from
Aug 13, 2019
Merged

Conversation

afontcu
Copy link
Member

@afontcu afontcu commented Aug 10, 2019

As suggested in testing-library/user-event#153, VTL mutating fireEvent from DOM Testing Library is giving us a hard time trying to make user-event usable in VTL. Also, I'd say that mutating an imported object isn't the safest strategy.

I've also added a bunch of tests for the "new" fireEvent.

This is not a breaking change. Actually, it is. fireEvent() now returns a Promise, and it didn't.

@afontcu afontcu requested a review from dfcook August 10, 2019 09:42
fireEvent[fn] = async (...params) => {
fireEvent[`_${fn}`](...params)
// Vue Testing Library's version of fireEvent will call DOM Testing Library's
// version of fireEvent plus wait for one tick of the event loop so that...
Copy link
Member Author

@afontcu afontcu Aug 10, 2019

Choose a reason for hiding this comment

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

"so that..."

couldn't come up with a good wording to explain why waiting for a tick is needed. Any ideas?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd love to know, too, whether if making fireEvent return a promise is totally necessary due to Vue internals. There might be some alternative that keeps VTL public API as similar as possible to RTL's and DTL's.

Choose a reason for hiding this comment

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

Any ideas?

How about "to allow vue to asynchronously handle the event"?

Maybe it would make sense to link to the docs here: https://vuejs.org/v2/guide/reactivity.html#Async-Update-Queue

Good question on if it is necessary for fireevent to be async. Both React and Preact expose ways of forcing them to flush their update queues synchronously. Maybe it is worth looking into vm.$forceUpdate to see if it does what we want

Copy link
Member Author

Choose a reason for hiding this comment

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

How about "to allow vue to asynchronously handle the event"?

Sounds good! Added.

I'm gonna give vm.$forceUpdate (or even nextTick? who knows) a go. I'd really love to stick to synchronous events.

Choose a reason for hiding this comment

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

Might also be worth looking into vuejs/vue-test-utils#1137

@codecov
Copy link

codecov bot commented Aug 10, 2019

Codecov Report

Merging #75 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master    #75   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           1      1           
  Lines          61     62    +1     
  Branches       10     10           
=====================================
+ Hits           61     62    +1
Impacted Files Coverage Δ
src/vue-testing-library.js 100% <100%> (ø) ⬆️

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 0e2aa0a...c6a231f. Read the comment docs.

@afontcu afontcu mentioned this pull request Aug 10, 2019
@testing-library testing-library deleted a comment from codecov bot Aug 10, 2019
@afontcu afontcu added the BREAKING CHANGE This change will require a major version bump label Aug 10, 2019
@afontcu
Copy link
Member Author

afontcu commented Aug 13, 2019

Thanks for the approval @dfcook! Merging this one. Are you publishing a new major version?

@afontcu afontcu merged commit f462051 into master Aug 13, 2019
@afontcu afontcu deleted the immutable-fireevent branch August 13, 2019 07:56
@dfcook
Copy link
Collaborator

dfcook commented Aug 13, 2019

2.0.0 released

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BREAKING CHANGE This change will require a major version bump released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants