Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

setInterval returns undefined after the window closes #2617

Closed
lacker opened this issue Jun 21, 2019 · 5 comments · Fixed by #2803
Closed

setInterval returns undefined after the window closes #2617

lacker opened this issue Jun 21, 2019 · 5 comments · Fixed by #2803

Comments

@lacker
Copy link

lacker commented Jun 21, 2019

Basic info:

Node 8
jsdom 15.1.1

Minimal reproduction case

const { JSDOM } = require("jsdom");

const dom = new JSDOM("");
let callback = () => {
  let value = dom.window.setInterval(() => {}, 1000);
  console.log("this should not be undefined:", value);
};
dom.window.close();
process.nextTick(callback);

The problem is that in jsdom, setInterval returns undefined after the window closes. However, normally setInterval will never return undefined, either in Node or in browsers. So it's reasonable for some code to be nested in a bunch of callbacks, and run after the jsdom window has been closed, but to expect a non-undefined return value for setInterval.

Specifically, some code in the simple-peer library does something like let returnValue = setInterval(...); if (returnValue.unref) { ... } to handle the case where timers should be unref'd to let Node exit. That code is correct according to either browsers or Node, but fails in a jsdom environment.

Instead, perhaps this could return 0 or some other number? That is normally a valid return value for setInterval.

How does similar code behave in browsers?

Edit: Chrome just returns 0. It is possible to call setInterval on a window after it closes, by calling it from another window.

@domenic
Copy link
Member

domenic commented Jun 21, 2019

We should probably make it (and maybe everything in jsdom) throw after the window closes.

@Sebmaster
Copy link
Member

Sebmaster commented Jun 21, 2019

@domenic I think this is actually a bug though.

x = window.open("about:blank")
x.setTimeout("document.body.innerText += 'hi'", 1000) // 1
x.close();
x.setTimeout("document.body.innerText += 'hi'", 1000) // 2

@lacker
Copy link
Author

lacker commented Jun 21, 2019

Actually I was wrong about browser behavior - you can call setInterval after the window closes, if you're doing it from the script that opened the window. In Chrome, setInterval appears to return 0 in this situation. Specifically this code:

let w = window.open();
w.close();
let returnValue = w.setInterval(() => {}, 1000);
console.log(returnValue);

logs 0. So I think setInterval and probably setTimeout as well should just return 0 rather than undefined in this situation.

@t-mullen
Copy link

Returning anything but a nonzero integer is against HTML spec.

https://html.spec.whatwg.org/multipage/timers-and-user-prompts.html#dom-setinterval
https://html.spec.whatwg.org/multipage/timers-and-user-prompts.html#timer-initialisation-steps

The setInterval() method must return the value returned by the timer initialization steps ...a user-agent-defined integer that is greater than zero that will identify the timeout to be set by this call in the list of active timers.

@lacker
Copy link
Author

lacker commented Dec 13, 2019

Chrome returns 0 after the window closes, though. So I guess Chrome is against HTML spec here? I would suggest just returning 0 for Chrome compatibility. I agree it should return some sort of integer.

domenic added a commit that referenced this issue Jan 18, 2020
This setup follows the spec more, is a lot simpler to follow, fixes #2800, and fixes #2617.
domenic added a commit that referenced this issue Jan 29, 2020
This setup follows the spec more, is a lot simpler to follow, fixes #2800, and fixes #2617.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants