waitForDomChange Implementation #118

alexwendte · 2018-10-10T01:08:41Z

What:
waitForDomChange is implemented along with tests, documentation, and types.
waitForElement is changed to require a callback function

Why:
Resolves issue number #111

How:
I copied much of what was in waitForElement. They are very similar

Checklist:

Documentation
Tests
Ready to be merged
Added myself to contributors table

…types.

sompylasar

LGTM overall, questions inside.

sompylasar · 2018-10-10T01:47:31Z

README.md

+  - [`waitForDomChange`](#waitfordomchange)
  - [`fireEvent`](#fireevent)
+    - [`fireEvent[eventName]`](#fireeventeventname)
+    - [`getNodeText`](#getnodetext)


Should the indentation be less on this line? getNodeText looks independent of fireEvent.

Yep, let's remove one of the # from getNodeText 👍 Do you mind doing that @themostcolm?

sompylasar · 2018-10-10T01:53:36Z

src/__tests__/wait-for-dom-change.js

+  await skipSomeTimeForMutationObserver()
+
+  expect(successHandler).toHaveBeenCalledTimes(1)
+  expect(successHandler).toHaveBeenCalledWith(true)


Why is it called with true?

Agreed, that's a good question. Why don't we have the success handler be called with the mutationsList which the mutation observer will give our callback?

This would be a good use case of toMatchSnapshot right? I was just copying the other ones from waitForElement, but this one would make sense to have I believe.

I'd prefer toMatchInlineSnapshot. If it's too big for that, then it's probably too big in general and we need a more specific assertion.

sompylasar · 2018-10-10T01:58:02Z

src/wait-for-dom-change.js

+    }
+    timer = setTimeout(onTimeout, timeout)
+    observer = new window.MutationObserver(onMutation)
+    observer.observe(container, mutationObserverOptions)


Should this MutationObserver code be reused somehow, not copied, between waitForElement and waitForDomChange?

I'd prefer if we could reuse the code 👍

waitForElement could probably wrap around waitForDomChange...

How do you propose we reuse the code?

kentcdodds

Thanks! A few comments/questions/requests :)

kentcdodds · 2018-10-10T02:19:09Z

README.md

+  - [`waitForDomChange`](#waitfordomchange)
  - [`fireEvent`](#fireevent)
+    - [`fireEvent[eventName]`](#fireeventeventname)
+    - [`getNodeText`](#getnodetext)


Yep, let's remove one of the # from getNodeText 👍 Do you mind doing that @themostcolm?

kentcdodds · 2018-10-10T02:20:46Z

README.md

-| [<img src="https://avatars2.githubusercontent.com/u/21689428?v=4" width="100px;"/><br /><sub><b>Jonathan Stoye</b></sub>](http://jonathanstoye.de)<br />[📖](https://github.com/kentcdodds/dom-testing-library/commits?author=JonathanStoye "Documentation") | [<img src="https://avatars2.githubusercontent.com/u/4126644?v=4" width="100px;"/><br /><sub><b>Sanghyeon Lee</b></sub>](https://github.com/yongdamsh)<br />[💡](#example-yongdamsh "Examples") | [<img src="https://avatars3.githubusercontent.com/u/8015514?v=4" width="100px;"/><br /><sub><b>Justice Mba </b></sub>](https://github.com/Dajust)<br />[💻](https://github.com/kentcdodds/dom-testing-library/commits?author=Dajust "Code") [📖](https://github.com/kentcdodds/dom-testing-library/commits?author=Dajust "Documentation") [🤔](#ideas-Dajust "Ideas, Planning, & Feedback") | [<img src="https://avatars3.githubusercontent.com/u/340761?v=4" width="100px;"/><br /><sub><b>Wayne Crouch</b></sub>](https://github.com/wgcrouch)<br />[💻](https://github.com/kentcdodds/dom-testing-library/commits?author=wgcrouch "Code") | [<img src="https://avatars1.githubusercontent.com/u/4996462?v=4" width="100px;"/><br /><sub><b>Ben Elliott</b></sub>](http://benjaminelliott.co.uk)<br />[💻](https://github.com/kentcdodds/dom-testing-library/commits?author=benelliott "Code") | [<img src="https://avatars3.githubusercontent.com/u/577921?v=4" width="100px;"/><br /><sub><b>Ruben Costa</b></sub>](http://nuances.co)<br />[💻](https://github.com/kentcdodds/dom-testing-library/commits?author=rubencosta "Code") | [<img src="https://avatars2.githubusercontent.com/u/4982001?v=4" width="100px;"/><br /><sub><b>Robert Smith</b></sub>](http://rbrtsmith.com/)<br />[🐛](https://github.com/kentcdodds/dom-testing-library/issues?q=author%3Arbrtsmith "Bug reports") [🤔](#ideas-rbrtsmith "Ideas, Planning, & Feedback") [📖](https://github.com/kentcdodds/dom-testing-library/commits?author=rbrtsmith "Documentation") |
-| [<img src="https://avatars3.githubusercontent.com/u/881986?v=4" width="100px;"/><br /><sub><b>dadamssg</b></sub>](https://github.com/dadamssg)<br />[💻](https://github.com/kentcdodds/dom-testing-library/commits?author=dadamssg "Code") | [<img src="https://avatars1.githubusercontent.com/u/186971?v=4" width="100px;"/><br /><sub><b>Neil Kistner</b></sub>](https://neilkistner.com/)<br />[💻](https://github.com/kentcdodds/dom-testing-library/commits?author=wyze "Code") | [<img src="https://avatars3.githubusercontent.com/u/1448597?v=4" width="100px;"/><br /><sub><b>Ben Chauvette</b></sub>](http://bdchauvette.net/)<br />[💻](https://github.com/kentcdodds/dom-testing-library/commits?author=bdchauvette "Code") | [<img src="https://avatars2.githubusercontent.com/u/777527?v=4" width="100px;"/><br /><sub><b>Jeff Baumgardt</b></sub>](https://github.com/JeffBaumgardt)<br />[💻](https://github.com/kentcdodds/dom-testing-library/commits?author=JeffBaumgardt "Code") [📖](https://github.com/kentcdodds/dom-testing-library/commits?author=JeffBaumgardt "Documentation") | [<img src="https://avatars0.githubusercontent.com/u/4658208?v=4" width="100px;"/><br /><sub><b>Matan Kushner</b></sub>](http://matchai.me)<br />[💻](https://github.com/kentcdodds/dom-testing-library/commits?author=matchai "Code") [📖](https://github.com/kentcdodds/dom-testing-library/commits?author=matchai "Documentation") [🤔](#ideas-matchai "Ideas, Planning, & Feedback") [⚠️](https://github.com/kentcdodds/dom-testing-library/commits?author=matchai "Tests") |
-
+| [<img src="https://avatars.githubusercontent.com/u/1500684?v=3" width="100px;"/><br /><sub><b>Kent C. Dodds</b></sub>](https://kentcdodds.com)<br />[💻](https://github.com/kentcdodds/dom-testing-library/commits?author=kentcdodds "Code") [📖](https://github.com/kentcdodds/dom-testing-library/commits?author=kentcdodds "Documentation") [🚇](#infra-kentcdodds "Infrastructure (Hosting, Build-Tools, etc)") [⚠️](https://github.com/kentcdodds/dom-testing-library/commits?author=kentcdodds "Tests") | [<img src="https://avatars1.githubusercontent.com/u/2430381?v=4" width="100px;"/><br /><sub><b>Ryan Castner</b></sub>](http://audiolion.github.io)<br />[📖](https://github.com/kentcdodds/dom-testing-library/commits?author=audiolion "Documentation")                                                                                                                                                                        | [<img src="https://avatars0.githubusercontent.com/u/8008023?v=4" width="100px;"/><br /><sub><b>Daniel Sandiego</b></sub>](https://www.dnlsandiego.com)<br />[💻](https://github.com/kentcdodds/dom-testing-library/commits?author=dnlsandiego "Code")                                                                                                                                                                                      | [<img src="https://avatars2.githubusercontent.com/u/12592677?v=4" width="100px;"/><br /><sub><b>Paweł Mikołajczyk</b></sub>](https://github.com/Miklet)<br />[💻](https://github.com/kentcdodds/dom-testing-library/commits?author=Miklet "Code")                                                                                                                                                                                                                                                          | [<img src="https://avatars3.githubusercontent.com/u/464978?v=4" width="100px;"/><br /><sub><b>Alejandro Ñáñez Ortiz</b></sub>](http://co.linkedin.com/in/alejandronanez/)<br />[📖](https://github.com/kentcdodds/dom-testing-library/commits?author=alejandronanez "Documentation")                                                                                                                                                                                           | [<img src="https://avatars0.githubusercontent.com/u/1402095?v=4" width="100px;"/><br /><sub><b>Matt Parrish</b></sub>](https://github.com/pbomb)<br />[🐛](https://github.com/kentcdodds/dom-testing-library/issues?q=author%3Apbomb "Bug reports") [💻](https://github.com/kentcdodds/dom-testing-library/commits?author=pbomb "Code") [📖](https://github.com/kentcdodds/dom-testing-library/commits?author=pbomb "Documentation") [⚠️](https://github.com/kentcdodds/dom-testing-library/commits?author=pbomb "Tests") | [<img src="https://avatars1.githubusercontent.com/u/1288694?v=4" width="100px;"/><br /><sub><b>Justin Hall</b></sub>](https://github.com/wKovacs64)<br />[📦](#platform-wKovacs64 "Packaging/porting to new platform")                                                                                                                                                                                                                               |


I think your editor messed some stuff up here... Could you make sure that the precommit hooks run so prettier will automatically format this properly?

You could also run npx kcd-scripts format before committing to be sure.

Don't forget to run:

npx kcd-scripts contributors generate npx kcd-scripts format

:)

Darn it. I ran it before and then when I went back in to change the README again I ran format and precommit but not generate.

kentcdodds · 2018-10-10T02:23:51Z

src/__tests__/wait-for-dom-change.js

+  expect(errorHandler).toHaveBeenCalledTimes(0)
+
+  document.body.appendChild(document.createElement('div'))
+  expect(document.body).toMatchSnapshot()


I don't think the snapshots are necessary.

kentcdodds · 2018-10-10T02:23:58Z

src/__tests__/wait-for-dom-change.js

+  expect(successHandler).toHaveBeenCalledTimes(1)
+  expect(successHandler).toHaveBeenCalledWith(true)
+  expect(errorHandler).toHaveBeenCalledTimes(0)
+  expect(document.body).toMatchSnapshot()


This one isn't necessary either.

kentcdodds · 2018-10-10T02:24:15Z

src/__tests__/wait-for-dom-change.js

+  expect(errorHandler).toHaveBeenCalledTimes(0)
+  expect(document.body).toMatchSnapshot()
+
+  return promise


We don't need to return this promise. Please remove this.

(because async functions always return promises and the return value of this one isn't used)

kentcdodds · 2018-10-10T02:25:15Z

src/__tests__/wait-for-dom-change.js

+  expect(errorHandler).toHaveBeenCalledTimes(0)
+  expect(container).toMatchSnapshot()
+
+  return promise


Remove this

kentcdodds · 2018-10-10T02:26:32Z

src/__tests__/wait-for-dom-change.js

+  await skipSomeTimeForMutationObserver()
+
+  expect(successHandler).toHaveBeenCalledTimes(1)
+  expect(successHandler).toHaveBeenCalledWith(true)


Agreed, that's a good question. Why don't we have the success handler be called with the mutationsList which the mutation observer will give our callback?

kentcdodds · 2018-10-10T02:26:40Z

src/__tests__/wait-for-dom-change.js

+  expect(successHandler).toHaveBeenCalledWith(true)
+  expect(errorHandler).toHaveBeenCalledTimes(0)
+
+  return promise


Remove this

kentcdodds · 2018-10-10T02:30:55Z

src/wait-for-dom-change.js

+    }
+    timer = setTimeout(onTimeout, timeout)
+    observer = new window.MutationObserver(onMutation)
+    observer.observe(container, mutationObserverOptions)


I'd prefer if we could reuse the code 👍

kentcdodds · 2018-10-10T02:32:09Z

src/wait-for-dom-change.js

+} = {}) {
+  return new Promise((resolve, reject) => {
+    // Disabling eslint prefer-const below: either prefer-const or no-use-before-define triggers.
+    let lastError, observer, timer // eslint-disable-line prefer-const


I'm pretty sure that if you remove observer and timer from here and use a const declaration below you should be fine :)

alexwendte · 2018-10-10T03:45:44Z

Yep, I’m happy to do these things! This is a great learning experience for me! I look forward to revisit everything and seeing what is conducive to good tests and nicely written code. Per the formatting. I installed doctoc and thought that would format everything in the read me correctly. I will use kcd-scripts format now.

kentcdodds · 2018-10-10T03:47:08Z

You may need to run npx kcd-scripts contributors generate first to get the table fixed up, and then run npx kcd-scripts format 👍

alexkrolick · 2018-10-10T05:27:32Z

It would be nice to see some tests around this in react-testing-library. Do all mutation events fire at the same time in a React render or could there be race conditions if you are expecting a mutation deeper in the tree?

alexwendte

I believe I implemented all of the requested features expect for making the mutation observer shared between waitForElement and waitForDomChange

…ectly formats README.md. Changes Promise to return a mutationsList. Adds documentation for mutationsList.

kentcdodds

Ok, I'm pretty happy with this. Anyone else want to weigh in?

gnapse · 2018-10-10T19:58:54Z

This does not look to be taking care of removing this functionality from waitForElement (and hence the problematic callback-less waitForElement form). Just checking. If it's going to be taken care of in a separate PR, that's fine too.

kentcdodds · 2018-10-10T20:05:50Z

Good point @gnapse.

@themostcolm, could you please do that as well. It's technically breaking a use case that we never expected/recommended so I'm not going to make it a major version bump.

alexwendte · 2018-10-10T21:16:19Z

Absolutely!

I didn't do it before because I thought it would be a breaking change, but what you said makes sense @kentcdodds

kentcdodds

I think this looks great other than the README formatting thing. What do you all think?

…hanges tests to account for new behavior. removes the documentation around the default callback. adds MutationObserver documentation for waitForDomChange

kentcdodds

Perfect 👍

alexwendte · 2018-10-11T15:51:18Z

Would you like me to update the branch and then squash everything into one commit, or do you would prefer separate commits?

kentcdodds · 2018-10-11T16:06:21Z

Nope, we're good. Thanks @alexwendte!

kentcdodds · 2018-10-11T16:07:06Z

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

alexwendte · 2018-10-11T23:04:17Z

Awesome, thanks so much Kent, I appreciate it! I’ve already learned a ton because of your generosity with recording workshops. I absolutely love the way you code, so I’m looking forward to getting to help out with what you’re working on!

kentcdodds · 2018-10-12T14:44:03Z

🎉 This PR is included in version 3.10.0 🎉

The release is available on:

npm package (@latest dist-tag)
GitHub release

Your semantic-release bot 📦🚀

waitForDomChange is implemented along with tests, documentation, and …

ba4a70e

…types.

sompylasar reviewed Oct 10, 2018

View reviewed changes

kentcdodds reviewed Oct 10, 2018

View reviewed changes

alexwendte commented Oct 10, 2018

View reviewed changes

Implements changes requested on 10-9-18. Removes matchSnapshots. Corr…

cdb4633

…ectly formats README.md. Changes Promise to return a mutationsList. Adds documentation for mutationsList.

alexwendte force-pushed the pr/wait-for-dom-change branch from 079bffb to cdb4633 Compare October 10, 2018 17:15

kentcdodds previously approved these changes Oct 10, 2018

View reviewed changes

alexwendte dismissed kentcdodds’s stale review via f972da8 October 10, 2018 22:45

kentcdodds previously approved these changes Oct 10, 2018

View reviewed changes

causes waitForElement to throw an error if callback is not defined. c…

35fa979

…hanges tests to account for new behavior. removes the documentation around the default callback. adds MutationObserver documentation for waitForDomChange

alexwendte dismissed kentcdodds’s stale review via 35fa979 October 10, 2018 23:10

alexwendte force-pushed the pr/wait-for-dom-change branch from f972da8 to 35fa979 Compare October 10, 2018 23:10

kentcdodds approved these changes Oct 10, 2018

View reviewed changes

kentcdodds merged commit 4cf93d2 into testing-library:master Oct 11, 2018

gnapse mentioned this pull request Oct 11, 2018

Make waitForElement always resolve to a truthy value. #111

Closed

waitForDomChange Implementation #118

waitForDomChange Implementation #118

Uh oh!

Conversation

alexwendte commented Oct 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sompylasar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kentcdodds left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexwendte commented Oct 10, 2018 via email • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kentcdodds commented Oct 10, 2018

Uh oh!

alexkrolick commented Oct 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexwendte left a comment

Choose a reason for hiding this comment

Uh oh!

kentcdodds left a comment

Choose a reason for hiding this comment

Uh oh!

gnapse commented Oct 10, 2018

Uh oh!

kentcdodds commented Oct 10, 2018

Uh oh!

alexwendte commented Oct 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

alexwendte commented Oct 10, 2018 •

edited

Loading

alexwendte commented Oct 10, 2018 via email •

edited

Loading

alexkrolick commented Oct 10, 2018 •

edited

Loading

alexwendte commented Oct 10, 2018 •

edited

Loading

alexwendte commented Oct 11, 2018 via email •

edited

Loading