Proposal: Sandbox setImmediate() in jsdom environment

[gaearon]

🚀 Feature Proposal

Currently, setTimeout and setInterval are sandboxed in the Jest jsdom environment.
Concretely, the Jest jsdom environment calls window.close() which will clear all timers:

If you want to be sure to shut down a jsdom window, use window.close(), which will terminate all running timers (and also remove any event listeners on the window and document).

However, setImmediate is not sandboxed like this.

The proposal is to sandbox setImmediate in the jsdom environment. Concretely, let’s clear all the pending immediates right after the window.close() call for consistency. This prevents running them at the time that DOM is already torn down, and we're in an invalid state.

To implement this, we would expose a wrapper that tracks the immediate IDs, removes IDs from the set when the immediate runs, and calls the underlying clearImmediate on the whole set when jsdom shuts down.

Motivation and Example

Jest source code explicitly calls this out as "for now it's OK" which signals to me that this was anticipated as a possible problem.

The motivation is that we want to start using setImmediate in more places, but Jest makes it difficult because by the time immediates run, the jsdom environment is already torn down.

This works (setTimeout is effectively ignored after jsdom shutdown):

test('foo', () => {
  setTimeout(() => {
    document.createElement('div');
  }, 0);
});

but this doesn't (setImmediate still runs after jsdom shutdown):

test('foo', () => {
  setImmediate(() => {
    document.createElement('div'); // crash: jsdom is shut down
  });
});

The proposal is to remove this inconsistency.

The proposal only concerns the Jest jsdom environment because it already sandboxes timers, and jsdom environment is useless after teardown. I don't think this is needed in a plain Node environment.

[SimenB]

I think this is a good idea 👍

I think ideally we'd throw errors on any work after test teardown (we currently throw on require/import() at least), including invoking scheduled work or scheduling any new work. However, if most timers today are just cleared out (like JSDOM's docs indicate) we should make sure that happens for all timers as you mention. Would be nice to handle that properly for both fake and real timers, but that's probably out of scope?

Making setImmediate behave the same as set{Timeout,Interval} is the right thing to do, regardless of whether we in the future want to do more to stop things happening after tests have been torn down.

---

EDIT: Most of ny reply here applies more to #11202, so I guess ignore the above! 😀 I 100% agree they should behave consistently, regardless of whether we want to throw, warn or do nothing. Since the "OG timers" ignore, we should replicate that behavior in setImmediate as well.

We probably shouldn't have setImmediate at all in JSDOM to be honest... From MDN:

This method is not expected to become standard, and is only implemented by recent builds of Internet Explorer and Node.js 0.10+. It meets resistance both from Gecko (Firefox) and Webkit (Google/Apple).

[gaearon]

I think ideally we'd throw errors on any work after test teardown (we currently throw on require/import() at least), including invoking scheduled work or scheduling any new work.

Yeah. I think of it as kind of a "Stage 2" to come after this. Maybe a config option / new default? It would be pretty disruptive to enforce this in the existing codebase. Note that even in this mode, it may not always possible to cancel stuff (e.g. third party code may schedule it). But maybe Jest could enforce that you jest.runAllPendingStuff() or whatever. Regardless, it's a more aggressive change and I was hoping to align behavior first.

Would be nice to handle that properly for both fake and real timers, but that's probably out of scope?

I think with manual control it's not as much of an issue because at least they won't run in a dead environment on their own. Which is a problem with setImmediate today.

We probably shouldn't have setImmediate at all in JSDOM to be honest...

So this is a bit thorny. It exists in Node and IE. It's unlikely more browsers would add it, but it probably doesn't hurt to have it? We're actually considering relying on it in React (when available) because it maps the closest to the semantics we need in the browser. Even though in practice we use a MessageChannel hack in a browser because nothing else supports setImmediate. But it would be nice if we could just rely on setImmediate in tests instead, especially considering issues with Node.js MessageChannel like facebook/react#20756.

[SimenB]

Yeah. I think of it as kind of a "Stage 2" to come after this. Maybe a config option / new default? It would be pretty disruptive to enforce this in the existing codebase.

Yep, I touched on that in #11202. I agree it probably needs to be configurable (and deffo not something we should do now).

Would be nice to handle that properly for both fake and real timers, but that's probably out of scope?

I think with manual control it's not as much of an issue because at least they won't run in a dead environment on their own. Which is a problem with setImmediate today.

Makes sense 👍

But it would be nice if we could just rely on setImmediate in tests instead, especially considering issues with Node.js MessageChannel like facebook/react#20756.

I don't think we should remove it now (mostly as v27 is already packed with breaking changes), but I do think we should in a future version. Nothing would stop React from choosing to extend the default JSDOM environment in the same way Jest does today (by just assigning the one. from Node into the global of the test), but I think out of the box Jest's DOM environment should be as realistic as possible so code that runs in a test does not explode at runtime in a real browser.

[gaearon]

Nothing would stop React from choosing to extend the default JSDOM environment in the same way Jest does today (by just assigning the one. from Node into the global of the test), but I think out of the box Jest's DOM environment should be as realistic as possible so code that runs in a test does not explode at runtime in a real browser.

React itself doesn't have a particular test-specific setup (e.g. even CRA is just one of many and is also losing relevance), so I don't know where we'd do that. I'm actually not sure what we'd use in this case. setTimeout is not quite the same, and MessageChannel (which does exist in the browser but not in jsdom) has different Node.js semantics which don't let the process exit.

[SimenB]

React itself doesn't have a particular test-specific setup (e.g. even CRA is just one of many and is also losing relevance), so I don't know where we'd do that.

I meant in a custom test environment: https://github.com/facebook/react/blob/1d1e49cfa453b58769e87c3c8d321024d58c948f/scripts/jest/config.base.js#L27-L28

[gaearon]

Ah. So my comment is about React projects, not React itself.

[gaearon]

I think another long-term option is if jsdom implements MessageChannel but in a way that auto-closes all ports on window close.

[gaearon]

We've discussed this more with the team.
Updated proposal:

Step 1: Sandbox setImmediate and clearImmediate. This could be a patch.
Step 2: Remove setImmediate and clearImmediate from the Jest jsdom environment. This is probably a major bump. You're right they don't make sense there in the first place.
Step 3: If you add MessageChannel to jsdom, please ensure that all its ports get closed when the window is closed. In other words, don't copy over MessageChannel from Node directly unless you have a fix for this.

I think this would satisfy our requirements and matches what you've been saying.

Does this sound good? What timeline can we think of for releasing Step 1? Or would you jump to Step 2 right away?

[SimenB]

I don't think it's necessarily worth it to spend time on sandboxing something we're gonna remove? We're currently in a pre-release for a new major, so we could just remove it right away, but I don't feel strongly about it.

If you're up for sending a PR sandboxing it and it would be helpful for React in their tests I'm fine with that and just mark it for removal later. If you don't want to use it we can just remove it now.

---

As for MessageChannel in JSDOM, I think we should wait for jsdom/jsdom#2448 (although somebody ™️ probably needs to send a PR over there as it doesn't seem like something the maintainers are gonna prioritize)

[gaearon]

To be clear, the concern here is not with React internal tests, but with tests of everyone using React with Jest today. We want to do a bugfix in React but it would effectively break a bunch of tests for people using jest+jsdom.

So our fix is dependent on the jest+jsdom fix. This is why I was hoping we could do the less aggressive fix in a patch (isolation) and then the more aggressive one in a major (removal). So that as many people get the update as possible.

[gaearon]

So that when we release React with a fix, we can say "just bump your jsdom environment" to get rid of the crashing tests.

[SimenB]

Aha! Gotcha. I'm happy to land a patch making it work better - regardless of whether we remove it later or not, the behavior you've suggested (clearing on teardown) makes sense to me.

Note that the change will only be part of Jest 27, but I do think people can use jest-environment-jsdom@27 with at least jest@26, although I haven't verified.

[gaearon]

Hm. If we're going to do a major anyway, then it seems like it makes sense to just remove it.

[SimenB]

We're currently on 27-next.5, I'm hopeful of releasing it as stable this month (OSS time is not looking good for the next few weeks, but Easter is coming up, so maybe 🤞).

But yes, I'd rather not attempt to do some git magic to get Lerna to not be confused by going back and making a new 26 release. React could release a custom env if you want to help users out that extra mile? Overriding setup should be enough unless I'm missing something. Messaging would be "install this env" instead of "install the latest env" which is almost as good of a deal 😀

[gaearon]

I think we can do this as an intermediate step later if the update to 27 is too hard for people.

[SimenB]

Sounds good 👍

Do you wanna send a PR removing it? If not I'll get to it this weekend or next week

[gaearon]

If you can do it I'd appreciate it.

[SimenB]

I opened a PR at #11222. Our example using @testing-library/react shows this change breaks it. I've reported at testing-library/dom-testing-library#914.

/cc @kentcdodds @eps1lon

[SimenB]

@gaearon do you need a release of this (it'd be a pre-release, but still)?

[gaearon]

Yes, that would be helpful.

[SimenB]

Cool, 27.0.0-next.6 is out 🙂

[SimenB]

FWIW, I hope to release it stable during or right after Easter. Gonna be stuck at home regardless, so should be able to find the time to wrap it up

[github-actions]

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.