feat(wait): wait will now also run your callback on DOM changes #415

kentcdodds · 2019-12-13T18:16:18Z

What: Make wait use a MutationObserver in addition to the previous behavior

Why: This way we can get rid of waitForDomChange (in a future release) and we drastically simplify the other async utilities. wait also gets a super-power of being able to re-run the callback faster than the interval when the DOM is changed, so this should speed up people's tests who are using wait today. Closes #376

How: Remove wait-for-expect (cc @lgandecki) because it wont work nicely with this approach. I also refactored the other utilities to build on top of wait (which is something I've wanted to do for a long time now).

Checklist:

Documentation added to the docs site
~~I've prepared a PR for types targeting DefinitelyTyped~~ (this should not be necessary. No APIs have changed)
Tests
Ready to be merged
Set lower timeout default

codesandbox-ci · 2019-12-13T18:17:03Z

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

Sandbox	Source
long-rgb-gxwp3	Configuration

kentcdodds · 2019-12-13T18:17:14Z

src/wait-for-element.js

@@ -1,71 +1,21 @@
-import {


It's easier to just look at the post-change version of this file: https://github.com/testing-library/dom-testing-library/blob/pr/wait-improvements/src/wait-for-element.js

kentcdodds · 2019-12-13T18:17:43Z

src/wait-for-element-to-be-removed.js

@@ -1,85 +1,48 @@
-import {


It's easier to just look at the post-change version of this file: https://github.com/testing-library/dom-testing-library/blob/pr/wait-improvements/src/wait-for-element-to-be-removed.js

kentcdodds · 2019-12-13T18:35:49Z

That's weird. I think the codesandbox thing is a fluke.

mwmcode · 2019-12-15T05:27:23Z

Hi Kent,

I was gonna post a suggestion/PR, but since this one is about wait, I will just leave it here:

Can we expose wait under a second alias: until or till? I think the code would read nicer:

  await till(() => expect(MockRedirect).toHaveBeenCalledWith({to: '/'}, {}));

I know we can alias our imports.. just thought the API would look nicer.. though it might not be worth introducing new API changes (at this time)

Cheers,

kentcdodds · 2019-12-15T06:01:31Z

Hi @mustafawm, thanks for the feedback. I do prefer to avoid aliases when possible. We have an alias for getQueriesForElement (it's within), but I'm not a super-fan of it. The reason I made the alias was because I thought the function name was kinda long, but I didn't want to make a breaking change to rename it.

I'm not sure there's enough justification to alias this function, but you certainly could do this in your own utils module.

Good luck!

mwmcode · 2019-12-15T06:05:43Z

Hey Ken, thanks for your quick reply.

Agree. tbh, I wasn't feeling strongly about it .. just thought I put it out there and see :)

Thanks for the utils module suggestion 🙏

lgandecki · 2019-12-15T09:08:12Z

Hey Kent,

Thanks for pinging me here and letting me know - it will be bittersweet to see the installs of wait-for-expect plummet but it's for a good reason - the tiny helper is usable on it's own outside of dom-testing scope so we shouldn't be adding more complexity to it, and the dom-testing could use some improvements.. :)

I also like the idea of simplifying the API. To be honest in a lot of projects I've seen and helped with, people didn't even bother to use waitForDomChange, wait "just worked". Optimizations (savings of maybe a few dozens of ms per test) were not worth the extra pause (and learning) to decide which of the wait* tools the programmer should use in a given situation.

I'm glad that the initial design we've came up with lives on. It's so intuitive and clean. My only regret is the default long timeout - I was basing it off what I knew from browser-based testing (selenium, chimp, cypress, etc). But in more of a low-level environment the timeout hardly ever needs to go that high, with the downside that once things break the tests take forever to fail. (In browser-based testing the test would maybe take 8 seconds to fail instead of 4 seconds to pass, but in integration we can jump from 100 ms to 4500 ms which is much more annoying). It brakes the flow. It might be worth pointing this out more explicitly in the docs, that people might want to consider to change the default, if they run lightweight tests. But, at the same time, this family of libraries is all based on "smart defaults" :)

Also - if you are going to do some breaking changes, this might be something to rethink, maybe?

afontcu

Looks like a sensible change! Not only we remove a dependency, we also set the stage to simplify the exposed API in a future major version.

I've added a few comments here and there, but looks great overall 👍

afontcu · 2019-12-15T09:04:23Z

src/wait-for-element-to-be-removed.js

+  // As the name implies, waitForElementToBeRemoved should check `present` --> `removed`
+  if (isRemoved(callback())) {
+    throw new Error(
+      'The callback function which was passed did not return an element or non-empty array of elements. waitForElementToBeRemoved requires that the element(s) exist before waiting for removal.',


nitpicking ^^ sry about that

Suggested change

'The callback function which was passed did not return an element or non-empty array of elements. waitForElementToBeRemoved requires that the element(s) exist before waiting for removal.',

'The callback function which was passed did not return an element or non-empty array of elements. waitForElementToBeRemoved requires that the element(s) exist(s) before waiting for removal.',

src/wait.js

kentcdodds · 2020-01-21T18:44:58Z

Maybe we should change the default timeout as part of this change as well. I think it makes more sense for it to be ~1 second. If someone's test is taking longer than that for a single wait then they probably aren't mocking something they should be (in the context of Cypress and other E2E situations, we could change the default for those environments).

What do folks think about that?

afontcu · 2020-01-21T18:58:13Z

+1 at setting a lower default value for the timeout – I've never found a case where a 4.5s timeout would make a (well-written and properly mocked) test pass while a lower one would make the same test fail.

I'm ok with 1s.

kentcdodds · 2020-03-03T16:40:06Z

Rebased master and added two commits. If folks could take a look at the last two commits that would be super duper. Thanks!

codecov · 2020-03-03T16:40:06Z

Codecov Report

Merging #415 into beta will not change coverage.
The diff coverage is 100%.

@@         Coverage Diff         @@
##           beta   #415   +/-   ##
===================================
  Coverage   100%   100%           
===================================
  Files        22     22           
  Lines       404    412    +8     
  Branches     95     99    +4     
===================================
+ Hits        404    412    +8

Impacted Files	Coverage Δ
src/pretty-dom.js	`100% <ø> (ø)`	⬆️
src/role-helpers.js	`100% <ø> (ø)`	⬆️
src/wait.js	`100% <100%> (ø)`	⬆️
src/config.js	`100% <100%> (ø)`	⬆️
src/wait-for-dom-change.js	`100% <100%> (ø)`	⬆️
src/wait-for-element.js	`100% <100%> (ø)`	⬆️
src/wait-for-element-to-be-removed.js	`100% <100%> (ø)`	⬆️
src/queries/label-text.js	`100% <0%> (ø)`	⬆️

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 6084f53...a00cc96. Read the comment docs.

kentcdodds · 2020-03-03T16:41:08Z

src/config.js

-  asyncUtilTimeout: 4500,
+  asyncUtilTimeout: 1000,


Just realized I amended the first of those two changes. So this is all that first change was.

afontcu · 2020-03-03T17:37:15Z

src/wait-for-dom-change.js

+// deprecated... TODO: remove this method. People should use wait instead
+// the reasoning is that waiting for just any DOM change is an implementation
+// detail. People should be waiting for a specific thing to change.


If we plan to remove this method in the next major release, should be add a warning so people know they are relying on a soon-to-be-gone method? Is it too invasive?

I'm kinda thinking that we should add the warning in the major version bump, and then remove it in the next one.

Closes #376

kentcdodds · 2020-03-04T17:02:45Z

I'm starting to think that instead of deprecating we should just remove it entirely what do people think?

To be clear, I'm talking about waitForDomChange and waitForElement.

waitForElement can be done really easily with wait and waitForDomChange is testing implementation details (and can be done really easily with wait as well.

Thoughts?

kentcdodds · 2020-03-04T17:12:46Z

Changed my mind. We'll just have a deprecation warning for this release. People will be less mad at us 😅

kentcdodds · 2020-03-04T18:02:22Z

🎉 This PR is included in version 7.0.0-beta.2 🎉

The release is available on:

npm package (@beta dist-tag)
GitHub release

Your semantic-release bot 📦🚀

Closes #376 Closes #416 BREAKING CHANGE: `waitForElement` is deprecated in favor of `find*` queries or `wait`. BREAKING CHANGE: `waitForDomChange` is deprecated in favor of `wait` BREAKING CHANGE: default timeout for async utilities is now 1000ms rather than 4500ms. This can be configured: https://testing-library.com/docs/dom-testing-library/api-configuration

kentcdodds commented Dec 13, 2019

View reviewed changes

kentcdodds force-pushed the pr/wait-improvements branch 3 times, most recently from b2619b6 to 2ceb1a0 Compare December 13, 2019 18:34

afontcu previously approved these changes Dec 15, 2019

View reviewed changes

kentcdodds added the BREAKING CHANGE This change will require a major version bump label Jan 21, 2020

kentcdodds dismissed afontcu’s stale review via d7789a4 March 3, 2020 16:40

kentcdodds force-pushed the pr/wait-improvements branch from 2ceb1a0 to d7789a4 Compare March 3, 2020 16:40

kentcdodds commented Mar 3, 2020

View reviewed changes

eps1lon approved these changes Mar 3, 2020

View reviewed changes

afontcu reviewed Mar 3, 2020

View reviewed changes

kentcdodds changed the base branch from master to next March 4, 2020 16:59

kentcdodds added 2 commits March 4, 2020 10:00

feat(wait): wait will now also run your callback on DOM changes

6105400

Closes #376

simplify wait implementation

5e08893

deprecate old async utils

6e793e0

kentcdodds force-pushed the pr/wait-improvements branch from d7789a4 to 6e793e0 Compare March 4, 2020 17:23

kentcdodds added 2 commits March 4, 2020 10:24

no-console is the worst

e7f7d11

address nit-pick 😉

4a68f39

add coverage for wait-for-element-to-be-removed

393ba2f

kentcdodds changed the base branch from next to beta March 4, 2020 17:49

kentcdodds force-pushed the beta branch from e879745 to 9526427 Compare March 4, 2020 17:49

add note about find queries being preferred

a00cc96

kentcdodds merged commit 5aa6386 into beta Mar 4, 2020

kentcdodds deleted the pr/wait-improvements branch March 4, 2020 17:58

kentcdodds added the released on @beta label Mar 4, 2020

kentcdodds restored the pr/wait-improvements branch March 12, 2020 21:31

kentcdodds deleted the pr/wait-improvements branch March 12, 2020 21:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(wait): wait will now also run your callback on DOM changes #415

feat(wait): wait will now also run your callback on DOM changes #415

kentcdodds commented Dec 13, 2019 •

edited

Loading

codesandbox-ci bot commented Dec 13, 2019 •

edited

Loading

kentcdodds Dec 13, 2019

kentcdodds Dec 13, 2019

kentcdodds commented Dec 13, 2019

mwmcode commented Dec 15, 2019 •

edited

Loading

kentcdodds commented Dec 15, 2019

mwmcode commented Dec 15, 2019

lgandecki commented Dec 15, 2019

afontcu left a comment

afontcu Dec 15, 2019

kentcdodds commented Jan 21, 2020

afontcu commented Jan 21, 2020

kentcdodds commented Mar 3, 2020

codecov bot commented Mar 3, 2020 •

edited

Loading

kentcdodds Mar 3, 2020

afontcu Mar 3, 2020

kentcdodds Mar 3, 2020

kentcdodds commented Mar 4, 2020

kentcdodds commented Mar 4, 2020

kentcdodds commented Mar 4, 2020

	'The callback function which was passed did not return an element or non-empty array of elements. waitForElementToBeRemoved requires that the element(s) exist before waiting for removal.',
	'The callback function which was passed did not return an element or non-empty array of elements. waitForElementToBeRemoved requires that the element(s) exist(s) before waiting for removal.',

feat(wait): wait will now also run your callback on DOM changes #415

feat(wait): wait will now also run your callback on DOM changes #415

Conversation

kentcdodds commented Dec 13, 2019 • edited Loading

codesandbox-ci bot commented Dec 13, 2019 • edited Loading

kentcdodds Dec 13, 2019

Choose a reason for hiding this comment

kentcdodds Dec 13, 2019

Choose a reason for hiding this comment

kentcdodds commented Dec 13, 2019

mwmcode commented Dec 15, 2019 • edited Loading

kentcdodds commented Dec 15, 2019

mwmcode commented Dec 15, 2019

lgandecki commented Dec 15, 2019

afontcu left a comment

Choose a reason for hiding this comment

afontcu Dec 15, 2019

Choose a reason for hiding this comment

kentcdodds commented Jan 21, 2020

afontcu commented Jan 21, 2020

kentcdodds commented Mar 3, 2020

codecov bot commented Mar 3, 2020 • edited Loading

Codecov Report

kentcdodds Mar 3, 2020

Choose a reason for hiding this comment

afontcu Mar 3, 2020

Choose a reason for hiding this comment

kentcdodds Mar 3, 2020

Choose a reason for hiding this comment

kentcdodds commented Mar 4, 2020

kentcdodds commented Mar 4, 2020

kentcdodds commented Mar 4, 2020

kentcdodds commented Dec 13, 2019 •

edited

Loading

codesandbox-ci bot commented Dec 13, 2019 •

edited

Loading

mwmcode commented Dec 15, 2019 •

edited

Loading

codecov bot commented Mar 3, 2020 •

edited

Loading