add fireEvent from dom-testing-library #48

jomaxx · 2018-04-10T02:56:02Z

What: add fireEvent

Why: #35

How: reexport from dom-testing-library

Checklist:

Documentation
Tests
Ready to be merged
Added myself to contributors table

Wait for testing-library/dom-testing-library#13 to be merged

jomaxx · 2018-04-10T03:10:02Z

@sompylasar the 4 events that I've come across that aren't handled in React the same way as other events are change, select, mouseenter, mouseleave.

change seems to be triggered by input events
select seems to be triggered by some custom react-select event
mouseenter is triggered by mouseover (Simulate.mouseEnter and Simulate.mouseLeave not working facebook/react#1297)
mouseleave is triggered by mouseout (Simulate.mouseEnter and Simulate.mouseLeave not working facebook/react#1297)

I haven't been able to get this working in jsdom so triggering the real events and falling back to synthetic events seem like the cleanest option

sompylasar · 2018-04-10T03:24:28Z

@jomaxx Got it, thanks.

kentcdodds

Thanks for this! It's great!

kentcdodds · 2018-04-10T03:38:28Z

src/index.js

@@ -18,4 +21,11 @@ function render(ui, {container = document.createElement('div')} = {}) {
  }
 }

-export {render, Simulate, wait}
+// fallback to synthetic events for DOM events that React doesn't handle
+;['change', 'select', 'mouseEnter', 'mouseLeave'].forEach(eventName => {


I prefer to give names to arrays rather than doing this.

const syntheticEvents = ['change', 'select', 'mouseEnter', 'mouseLeave'] syntheticEvents.forEach(eventName => { document.addEventListener(eventName.toLowerCase(), e => { Simulate[eventName](e.target, e) }) })

Notice that I used document rather than window. I'm not certain whether that makes much of a difference in practice, but I checked the React source and I'm pretty sure that's where they attach event handlers.

kentcdodds · 2018-04-10T03:44:02Z

src/index.js

-function render(ui, {container = document.createElement('div')} = {}) {
+function render(
+  ui,
+  {container = document.body.appendChild(document.createElement('div'))} = {},


We definitely don't want to do this right now. We could expose a renderIntoDocument method that does this (and removes the node on unmount or something). The problem with this is that you could have a bunch of elements dangling out on the body throughout your tests and if you don't clean it up it could break things in unexpected ways. Also it's a source of memory leaks.

As discussed in #35 we'll have to recommend to people (in the docs) that if they want to use this method, they must either 1) append the container to the body themselves or 2) use the renderIntoDocument (which I think we should create as a small abstraction over this function which appends the container to the body). In either case people should be warned that they should create a beforeEach(() => document.body.innerHTML = '') in their test. Perhaps we could even create a util so they could do: beforeEach(clearDOM).

Thoughts?

Works for me. I'd suggest an unmountAll helper:

afterEach(unmountAll)

Clearing the DOM will clean up the DOM but won't trigger componentWillUnmount on components which might have their own cleanup to do.

maybe components cleaning up subscribers should not be our concern though

kentcdodds · 2018-04-10T03:46:04Z

Could you also update the package.json so the version of dom-testing-library is forced as part of this update?

codecov · 2018-04-10T04:11:27Z

Codecov Report

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

@@          Coverage Diff          @@
##           master    #48   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           1      1           
  Lines           6     18   +12     
=====================================
+ Hits            6     18   +12

Impacted Files	Coverage Δ
src/index.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 6d9c368...404e14b. Read the comment docs.

kentcdodds

Could you add the utilities we discussed renderIntoDocument and clearDocument with docs (for tests you can probably just use both of them in your fireEvent tests). I think we should just recommend people do beforeEach(clearDocument) in any test that they use renderIntoDocument. We should also add to the fireEvent docs that your components must be in the document for fireEvent to work, so we recommend people use renderIntoDocument and clearDocument if they want to use fireEvent. We should indicate also the benefit of using fireEvent over Simulate. Specifically that it's less react-specific and therefore aligns with the guiding principle more.

Thanks!

kentcdodds · 2018-04-10T04:30:26Z

src/index.js

@@ -18,4 +18,12 @@ function render(ui, {container = document.createElement('div')} = {}) {
  }
 }

-export {render, Simulate, wait}
+// fallback to synthetic events for DOM events that React doesn't handle


Shouldn't this be "React events that the DOM doesn't support" ?

kentcdodds

Getting closer! This is going to be great. Thanks!

kentcdodds · 2018-04-10T12:49:33Z

src/__tests__/renderIntoDocument.js

@@ -0,0 +1,16 @@
+import React from 'react'


Due to issues with operating systems, filename case sensitivity, and git, I prefer all my files to use kebab-casing rather than camelCasing. Could you rename this file please? I really should have an eslint rule for it.

kentcdodds · 2018-04-10T12:50:12Z

src/__tests__/renderIntoDocument.js

+it('renders button into document', () => {
+  const ref = React.createRef()
+  renderIntoDocument(<div id="test" ref={ref} />)
+  expect(document.body.querySelector('#test')).toBe(ref.current)


Could remove the id and the assertion could be: expect(document.body.firstChild).toBe(ref.current)

kentcdodds · 2018-04-10T12:51:32Z

src/index.js

+  document.body.innerHTML = ''
+}
+
+// fallback to synthetic events for DOM events that React doesn't handle


This comment needs an update I think. Unless I'm misunderstanding. It should say:

// fallback to synthetic events for React events that the DOM doesn't support

kentcdodds · 2018-04-10T12:52:42Z

README.md

@@ -313,6 +314,30 @@ The default `interval` is `50ms`. However it will run your callback immediately
 on the next tick of the event loop (in a `setTimeout`) before starting the
 intervals.



Let's update the docs here to document renderIntoDocument and clearDocument (in that order). Then update the fireEvent docs to explain why it's important to use those helpers.

oh i just added this, but added them below render and before Simulate

kentcdodds · 2018-04-10T12:53:49Z

README.md

+
+#### `fireEvent[eventName](node: HTMLElement, eventInit)`
+
+Convenience methods for firing DOM events. Look [here](./src/events.js) for full list.


Could you update this to say:

Convenience methods for firing DOM events. Check out [dom-testing-library/src/events.js](https://github.com/kentcdodds/dom-testing-library/blob/master/src/events.js) for a full list as well as default `eventProperties`.

kentcdodds · 2018-04-10T12:54:07Z

README.md

+)
+```
+
+#### `fireEvent[eventName](node: HTMLElement, eventInit)`


Could you update this to be:

#### `fireEvent[eventName](node: HTMLElement, eventProperties: Object)`

kentcdodds · 2018-04-10T12:54:52Z

README.md

+
+Convenience methods for firing DOM events. Look [here](./src/events.js) for full list.
+
+```javascript


Please update this example to the same in dom-testing-library:

// <button>Submit</button> const rightClick = {button: 2} fireEvent.click(getElementByText('Submit'), rightClick) // default `button` property for click events is set to `0` which is a left click.

kentcdodds

This is great 👍 Just a few questions.

kentcdodds · 2018-04-10T13:17:25Z

README.md

+  fireEvent.click(getElementByText('Submit'), rightClick)
+  // default `button` property for click events is set to `0` which is a left click.
+
+  // don't forget to unmount component so componentWillUnmount can clean up subscriptions


Is this totally required for react to do its job? I'm not certain it's necessary 🤔

If it is then perhaps our clearDocument should actually be called cleanup and do that job for people. If it's required for everyone to do it, and it's not confusing or complicated for us to do it in the library, then we should do it in the library.

ok we can do it in the library, but i think this is even more reason to have cleanup be in an afterEach hook so if a componentWillUnmount throws an error it will be associated with the correct test.

done but kept beforeEach in examples

ok we can do it in the library, but i think this is even more reason to have cleanup be in an afterEach hook so if a componentWillUnmount throws an error it will be associated with the correct test.

This makes a lot of sense. Sorry, but could you change it back? 😅

haha np, will change it back

kentcdodds · 2018-04-10T13:17:50Z

README.md

+import { renderIntoDocument, clearDocument, render, fireEvent }
+
+// don't forget to clean up the document.body
+afterEach(clearDocument)


I prefer to do this in beforeEach

gnapse · 2018-04-10T13:23:39Z

README.md

+
+test('clicks submit button', () => {
+  const spy = jest.fn();
+  const { unmount, getByText } render(<button onClick={spy}>Submit</button>)


The = is missing in this assignment.

Also, shouldn't this example be using renderIntoDocument instead of render?

antsmartian · 2018-04-10T16:19:19Z

@jomaxx This is awesome! Great to see -- we support so much in this small library! Cheers 💯

kentcdodds

I love this. Thank you so much for making this happen!

kentcdodds · 2018-04-10T16:33:47Z

Thanks so much for your help! I've added you as a collaborator on the project. Please make sure that you review the other/MAINTAINING.md and CONTRIBUTING.md files (specifically the bit about the commit messages and the git hooks) and familiarize yourself with the code of conduct (we're using the contributor covenant). You might also want to watch the repo to be notified when someone files an issue/PR. Please continue to make PRs as you feel the need (you can make your branches directly on the repo rather than your fork if you want). Thanks! And welcome to the team :)

* chore: update contributors * chore(readme): update to reflect the api

…d-8.0.4 Update lint-staged to the latest version 🚀

Josef Blake added 3 commits April 9, 2018 22:52

add fireEvent from dom-testing-library

8a10c39

add contributor

2fda80f

added docs

108e295

jomaxx mentioned this pull request Apr 10, 2018

added fireEvent util testing-library/dom-testing-library#13

Merged

4 tasks

kentcdodds requested changes Apr 10, 2018

View reviewed changes

use document for synthetic events

7f7daa6

kentcdodds requested changes Apr 10, 2018

View reviewed changes

added renderIntoDocument and clearDocument

2381249

kentcdodds requested changes Apr 10, 2018

View reviewed changes

Josef Blake added 5 commits April 10, 2018 09:03

added docs

6cbbce9

update tests and README.md

cc2310b

fix typos

eb482e7

fix typo

32f7560

update readme

8304913

kentcdodds reviewed Apr 10, 2018

View reviewed changes

gnapse reviewed Apr 10, 2018

View reviewed changes

Josef Blake added 4 commits April 10, 2018 09:33

fix typo

9006f0e

use beforeEach

04fec2e

added cleanup, removed clearDocument

a85fad9

use afterEach for cleanup

e210c0c

Update README.md

404e14b

kentcdodds approved these changes Apr 10, 2018

View reviewed changes

kentcdodds merged commit 86d3508 into testing-library:master Apr 10, 2018

kentcdodds mentioned this pull request Apr 10, 2018

Explore alternative event dispatch/simulation #35

Closed

julienw pushed a commit to julienw/react-testing-library that referenced this pull request Dec 20, 2018

chore: update documentation (testing-library#48)

7aed39f

* chore: update contributors * chore(readme): update to reflect the api

lucbpz pushed a commit to lucbpz/react-testing-library that referenced this pull request Jul 26, 2020

Merge pull request testing-library#48 from Gpx/greenkeeper/lint-stage…

c77ec95

…d-8.0.4 Update lint-staged to the latest version 🚀

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add fireEvent from dom-testing-library #48

add fireEvent from dom-testing-library #48

jomaxx commented Apr 10, 2018 •

edited

Loading

jomaxx commented Apr 10, 2018

sompylasar commented Apr 10, 2018

kentcdodds left a comment

kentcdodds Apr 10, 2018

kentcdodds Apr 10, 2018

jomaxx Apr 10, 2018

jomaxx Apr 10, 2018

kentcdodds commented Apr 10, 2018 •

edited

Loading

codecov bot commented Apr 10, 2018 •

edited

Loading

kentcdodds left a comment

kentcdodds Apr 10, 2018

kentcdodds left a comment

kentcdodds Apr 10, 2018

kentcdodds Apr 10, 2018

kentcdodds Apr 10, 2018

kentcdodds Apr 10, 2018

jomaxx Apr 10, 2018

kentcdodds Apr 10, 2018

kentcdodds Apr 10, 2018

kentcdodds Apr 10, 2018

kentcdodds left a comment

kentcdodds Apr 10, 2018

jomaxx Apr 10, 2018

jomaxx Apr 10, 2018

kentcdodds Apr 10, 2018

jomaxx Apr 10, 2018

jomaxx Apr 10, 2018

kentcdodds Apr 10, 2018

gnapse Apr 10, 2018

antsmartian commented Apr 10, 2018

kentcdodds left a comment

kentcdodds commented Apr 10, 2018

		@@ -313,6 +314,30 @@ The default `interval` is `50ms`. However it will run your callback immediately
		on the next tick of the event loop (in a `setTimeout`) before starting the
		intervals.


		#### `fireEvent[eventName](node: HTMLElement, eventInit)`

		Convenience methods for firing DOM events. Look [here](./src/events.js) for full list.


		Convenience methods for firing DOM events. Look [here](./src/events.js) for full list.

		```javascript

add fireEvent from dom-testing-library #48

add fireEvent from dom-testing-library #48

Conversation

jomaxx commented Apr 10, 2018 • edited Loading

jomaxx commented Apr 10, 2018

sompylasar commented Apr 10, 2018

kentcdodds left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kentcdodds commented Apr 10, 2018 • edited Loading

codecov bot commented Apr 10, 2018 • edited Loading

Codecov Report

kentcdodds left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kentcdodds left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kentcdodds left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antsmartian commented Apr 10, 2018

kentcdodds left a comment

Choose a reason for hiding this comment

kentcdodds commented Apr 10, 2018

jomaxx commented Apr 10, 2018 •

edited

Loading

kentcdodds commented Apr 10, 2018 •

edited

Loading

codecov bot commented Apr 10, 2018 •

edited

Loading