Upgrade wait-for-expect package

[brapifra]

Related to TheBrainFamily/wait-for-expect#20

[kentcdodds]

Will this be a breaking change? If so, I'll need to understand the justification for bumping the major version here and in all other consuming libraries for this change.

[brapifra]

It's a breaking change for those who are using the lib and are setting jest's timers to fake. This scenario used to work because wait-for-expect was actually using jest's default real timers.
That is obviously an undesired behaviour. The library should always use the current setted timers (e.g if they are set to fake, it should use the fake timers).

However, as we discussed in TheBrainFamily/wait-for-expect#20, it doesn't really make sense to use fake timers with wait-for-expect, so bumping the major version here should be fine.

Regarding the benefits, upgrading the package will allow to have fake timers as default and set them to real in specific test files. @kentcdodds

[kentcdodds]

Hmmm... I wonder if our own wait* utilities should receive this same treatment at the same time. Care to take a look and see if that's necessary/fix it?

• https://github.com/testing-library/dom-testing-library/blob/master/src/wait-for-element.js
• https://github.com/testing-library/dom-testing-library/blob/master/src/wait-for-element-to-be-removed.js
• https://github.com/testing-library/dom-testing-library/blob/master/src/wait-for-dom-change.js

[brapifra]

Sure. I will take a look at it tomorrow

[brapifra]

Indeed, the wait* utilities needed the same treatment. I've added some changes and unit tests to make sure they have the correct behaviour.

[kentcdodds]

I'd like to hear what other maintainers have to say about this breaking change.

To help clarify things, @brapifra, could you please give us a small example of a test that works today, but wont work with these changes, as well as one which does not work today and will work with these changes and why that is desirable?

[alexkrolick]

To help clarify things, @brapifra, could you please give us a small example of a test that works today, but wont work with these changes, as well as one which does not work today and will work with these changes and why that is desirable

Right... I think I expect this part of the code not to care about Jest or fake timers at all.

[brapifra]

@alexkrolick That part of the code does take care about Jest timers because is using the globals setTimeout/clearTimeout/.... These globals are mocked if jest timers are set to fake.

@kentcdodds This is an example that doesn't work today (when it should):

import { waitForElement } from "@testing-library/dom";

// The timers have been defaulted to fake in package.json/jest.config.json

describe("Suite", () => {
  it("should not timeout", async () => {
    jest.useRealTimers();

    const container = document.createElement("div");
    await waitForElement(() => container.querySelector("div"));
  }); // throws timeout error
});

It doesn't work because the dom-testing-library doesn't get all these global timeout variables lazily. Therefore, the unit test times out as it's using fake timers when it should use the real ones that have been set inside the it block.

On the other hand, this is working now (although it shouldn't) and it will start failing if you accept my changes:

import { waitForElement } from "@testing-library/dom";

// The timers are real by default if you don't change them in package.json/jest.config.json

describe("Suite", () => {
 it("should timeout", async () => {
   jest.useFakeTimers();

   const container = document.createElement("div");
   await waitForElement(() => container.querySelector("div"));
 }); // it passes
});

Due to the same reason, it's using the default real timers instead of the fake ones set inside the it block.

[kentcdodds]

Thank you for those examples. They're very helpful.

I think I've never run into this problem because I don't enable fake timers by default. Furthermore I also don't mix fake timers and the wait utilities in a single test (which I think we should recommend in the docs if we don't already).

So what we're doing here is changing the default I guess. When we do something like that, we need to consider if we accept and release this pull request:

1. The negative impact and the likelihood of that impact
2. The positive impact and the likelihood of that impact

Here's my analysis, I'd like to hear from others:

The negative impact and the likelihood of that impact: people who are mixing fake timers and wait utilities will have their tests break. I can't tell how many people this is, but it's probably a nontrivial amount of people, so I would say the likelihood of that impact is pretty high. In addition, the likelihood that people will continue to be impacted over time as they bump into this is nontrivial as well and the cost of teaching people to avoid this problem is nontrivial as well.

The positive impact and the likelihood of that impact: people who are using fake timers by default and using the wait utilities will be impacted. The number of people in that category today is 0. We know this because doing that is currently impossible. There may be a handful of people who want to do that, but I think that is actually pretty rare because the default configuration in Jest is to use real timers and the vast majority of people do not change the default because they either don't know it's possible, or don't know why they would want to. So the positive impact will be for people who know that they can use fake timers by default and configure jest to do so. I'd say that the impact is relatively low, but the likelihood of a positive impact for that smaller group is high.

Ultimately, mixing fake timers and our wait utilities is pretty problematic. I definitely would like it if we could make enabling fake timers by default work, but I don't want to force people to have to call jest.useRealTimers() every time they want to use an async utility. That would just be madness. So if that's what you have to do when you use fake timers by default, then we can't recommend enabling fake timers by default.

The fact is that async stuff (which is tested by our wait utilities) happens in an app way more often than people needing fake timers.

So here's what I want: Can we think of a way to make this work:

import { waitForElement } from "@testing-library/dom";

// The timers have been defaulted to fake in package.json/jest.config.json

describe("Suite", () => {
  it("should not timeout", async () => {
    // jest.useRealTimers(); // <-- this is not needed

    const container = document.createElement("div");
    await waitForElement(() => container.querySelector("div"));
  }); // it passes
});

Because I don't think that I want to make this change for what is a relatively small group of people and incur the negative impact on what is probably a relatively larger group of people (today and in the future) who will run into issues if we merge this PR.

[alexkrolick]

@kentcdodds that's what I meant, too - can we make wait never use fake timers at all. Fake timers are meant to control "app time" not so much the constructs that run in the test env.

[brapifra]

I agree with you that ideally this lib and others like wait-for-expect shouldn't depend on whether the timers have been mocked or not. However, as far as I know, that's not possible.

It's basically the same thing as having a library that is run in the browser: If someone else modifies any of the global variables that your library is using, it will probably stop working as expected.

And unfortunately, I'm one of the few that has a big project with fake timers by default 😞

EDIT: One option I thought about is to create a separate jsdom environment and use the window object from that environment:

import { JSDOM } from 'jsdom';

const globalObj = new JSDOM().window;

There would still be a point of failure (the jsdom module can also be mocked with jest.mock('jsdom');), but it is definitely less likely than the timers being set to fake.

EDIT2: Another solution would be to check if the timers have been set to fake by jest. In that case we could temporarily set them back to real in order to get the proper timeout functions:

const usingJestFakeTimers = window.setTimeout._isMockFunction;

// Disable jest's fake timers temporarily
if (usingJestFakeTimers) {
  jest.useRealTimers();
}

const globalObj = typeof window === 'undefined' ? global : window

...

const clearTimeoutFn = globalObj.clearTimeout
// istanbul ignore next
const setImmediateFn = globalObj.setImmediate || setImmediatePolyfill
const setTimeoutFn = globalObj.setTimeout

// Restore jest's timers
if (usingJestFakeTimers) {
  jest.useFakeTimers();
}

I like this solution more, although it would only fix the problem in jest. There would still be the case where the library is used with some other test runner that mocks the timers in a different way.

[brapifra]

Any thoughts about my previous comment? @kentcdodds @alexkrolick

[kentcdodds]

Whoa, I didn't get notified of your edit. Sorry about that. Will read it over soon.

[kentcdodds]

I'm in favor of your last solution of enabling fake timers. Let's solve for the jest case, and if others crop up we can deal with those. Just make sure that the solution code runs in other environments.

I love this. Thanks for putting some thought into it!

Could you make that happen?

[brapifra]

Could you make that happen?

I can, and I will... tomorrow 😄 I agree with you, the last solution is probably the best one.

[kentcdodds]

Unfortunately, because the wait-for-expect package implemented the change that it did, we will no longer be receiving updates because I don't want to accept the change that was made over there. For that reason, I think we'll leave things at version 1 and if there are changes needed, we'll probably just remove the dependency and write it ourselves for this library (unless the change there is reverted).

[brapifra]

I was planning to apply the same logic to wait-for-expect and open a PR in their repo. But I think it still makes sense to fix your own wait* upgrade functions and upgrade the wait-for-expect package once (if) they accept my change.

[kentcdodds]

But I think it still makes sense to fix your own wait* upgrade functions

Oh? I thought that all we needed to do was your suggestion above and that would result in no breaking change correct?

[brapifra]

Oopss... yeah, that's what I was trying to say 😅 By adding my suggestion, all the wait* functions will work as expected without any breaking change.

[kentcdodds]

Sweet! Thanks for talking us through this! I'll look forward to your updates when you get the chance :)

[brapifra]

There you go @kentcdodds. I have added a few more tests to make sure that the library restores the jest timers after getting the time functions.
Let me know what you think

EDIT: I'm working on improving the tests to reach 100% code coverage
EDIT2: Turns out that the newMutationObserver function also depends on setTimeout. I will make a few changes later.

[brapifra]

Ok, I think everything is properly changed and tested now. I've ended up creating a runWithRealTimers helper function that can be used whenever there is some piece of code that needs to be run without being affected by the current jest timers. It could also include some logic for other tests runners if needed in the future. @kentcdodds

[kentcdodds]

Thanks! This looks good to me 👍 Just a few comments.

[brapifra]

Hmm... the nodev8 build job failed. That's weird 🤔

I've just tried to run the tests with a couple of nodev8 versions that I had in my laptop and they passed 😕

[kentcdodds]

It could be a race condition issue. That sometimes happens with these lower-level async tests 😬

[kentcdodds]

This looks good to me. I'd love to get a 👍 from another maintainer on this though.

[brapifra]

Could you take a look at this? @alexkrolick

[kentcdodds]

I think this is fine. Let's get this merged 👍

[kentcdodds]

@all-contributors please add @brapifra for code and tests

[allcontributors]

@kentcdodds

I've put up a pull request to add @brapifra! 🎉

[kentcdodds]

Once wait-for-expect supports this, then I'd be happy to upgrade to that version :)

[kentcdodds]

🎉 This PR is included in version 6.3.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[lgandecki]

Hey guys, wait-for-expect got bumped to 3.0.0 to accommodate the changes in line with the work/discussion here.
TheBrainFamily/wait-for-expect#22

@kentcdodds feel free to take a look and decide if you want to upgrade.
I'd love that package to stay in line with what @testing-library does, it's great to have the same API/functionality no matter if someone uses the package for client-side, or for server-side code. I thought you were fine with the change that bumped us to 2.0 but reading that discussion again there was never a stamp of approval from you there, so my bad :) I had similar worries - that it would break for some people, while enabling people that didn't have a way of using it yet anyway, and didn't seem to complain. I'm happy you guys found out the way to do it without breaking anything.