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

'setTimeout is not defined' ReferenceError in CommonJS-like environment (Firefox SDK) #2177

Closed
andyoxus opened this Issue Mar 27, 2015 · 28 comments

Comments

Projects
None yet
7 participants
@andyoxus

andyoxus commented Mar 27, 2015

This is an unusual use-case of jQuery (2.1.3) - an SDK addon has no obvious window object, and no reason to manipulate the DOM (injected 'content scripts' do this). In fact there is a 'hidden' window, which means jQuery should be usable for things like Deferreds, etc. Here's the 'main' index.js (the first three lines are the Firefox 'magic' to get hold of the hidden window, and are not important):

var {Cc, Ci} = require("chrome");
var hiddenDOMWindow = Cc["@mozilla.org/appshell/appShellService;1"].
    getService(Ci.nsIAppShellService).hiddenDOMWindow;

var j = require('./jquery')(hiddenDOMWindow);

var deferred = j.Deferred().done(function(value) {
   console.log('Done', value);
});

deferred.resolve("hello world");

This throws a 'setTimeout is not defined' ReferenceError while require()ing jQuery, with the following stack trace

jQuery.ready.promise@resource://jq-issue/jquery.js:3441:4
resource://jq-issue/jquery.js:3456:1
module.exports@resource://jq-issue/jquery.js:31:12
resource://jq-issue/index.js:5:9
run@resource://gre/modules/commonjs/sdk/addon/runner.js:145:19
[...snip...]

Line 3441 of jquery.js is

setTimeout( jQuery.ready );

In this environment this is Sandbox, which doesn't have a setTimeout. window, however, does, and the fix (for me) is just

window.setTimeout( jQuery.ready );

There are other instances of this usage of setTimeout in jQuery.

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Mar 27, 2015

Member

The problem in changing setTimeout to window.setTimeout is that if you want to use pieces of jQuery in another environment that doesn't have window, i.e. Node.js then it won't work. So I don't think we can do that. The only way I can see that'd cover both scenarios would be to create a hook for users to provide their own setTimeout/etc. implementations and just default to global.setTimeout.

The list of assumed globals can be found at:

jquery/src/.jshintrc

Lines 17 to 20 in bd9a138

"setTimeout": true,
"clearTimeout": true,
"setInterval": true,
"clearInterval": true,

(window & noGlobal are taken from the factory, define & module are for AMD/CommonJS). So this problem affects only timers.

Member

mgol commented Mar 27, 2015

The problem in changing setTimeout to window.setTimeout is that if you want to use pieces of jQuery in another environment that doesn't have window, i.e. Node.js then it won't work. So I don't think we can do that. The only way I can see that'd cover both scenarios would be to create a hook for users to provide their own setTimeout/etc. implementations and just default to global.setTimeout.

The list of assumed globals can be found at:

jquery/src/.jshintrc

Lines 17 to 20 in bd9a138

"setTimeout": true,
"clearTimeout": true,
"setInterval": true,
"clearInterval": true,

(window & noGlobal are taken from the factory, define & module are for AMD/CommonJS). So this problem affects only timers.

@andyoxus

This comment has been minimized.

Show comment
Hide comment
@andyoxus

andyoxus Mar 28, 2015

Understood. A naive solution is to fall back to (the local) window.setTimeout (and other timer functions) if the global setTimeout doesn't exist. But that of course breaks the concept of assumed globals.

I guess the Firefox SDK is pretty much unique in being CommonJS, but not having these globals available. And in fact it's perhaps more a Firefox issue.

andyoxus commented Mar 28, 2015

Understood. A naive solution is to fall back to (the local) window.setTimeout (and other timer functions) if the global setTimeout doesn't exist. But that of course breaks the concept of assumed globals.

I guess the Firefox SDK is pretty much unique in being CommonJS, but not having these globals available. And in fact it's perhaps more a Firefox issue.

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Mar 28, 2015

Member

Understood. A naive solution is to fall back to (the local) window.setTimeout (and other timer functions) if the global setTimeout doesn't exist.

That could work. We already expose some of the globals as variables, see:
https://github.com/jquery/jquery/blob/bd9a1385feea349f48de658875c3f1bfc75daf74/src/var/documentElement.js
See also the commit that made our code stop assuming the browser environment:
76df9e4

We could even check for window.setTimeout first and fallback to setTimeout which would make things work a little better for jsdom which exposes patched setTimeout under its window to be able to cancel all timeouts on window.close:
https://github.com/tmpvar/jsdom/blob/c59ad9a8f20508fc2b199195891ef62999ba7b8c/lib/jsdom/browser/Window.js#L190

This all would add some bytes, though so if it could be fixed on the Firefox SDK side it'd be better.

@jquery/core, what do you think?

Member

mgol commented Mar 28, 2015

Understood. A naive solution is to fall back to (the local) window.setTimeout (and other timer functions) if the global setTimeout doesn't exist.

That could work. We already expose some of the globals as variables, see:
https://github.com/jquery/jquery/blob/bd9a1385feea349f48de658875c3f1bfc75daf74/src/var/documentElement.js
See also the commit that made our code stop assuming the browser environment:
76df9e4

We could even check for window.setTimeout first and fallback to setTimeout which would make things work a little better for jsdom which exposes patched setTimeout under its window to be able to cancel all timeouts on window.close:
https://github.com/tmpvar/jsdom/blob/c59ad9a8f20508fc2b199195891ef62999ba7b8c/lib/jsdom/browser/Window.js#L190

This all would add some bytes, though so if it could be fixed on the Firefox SDK side it'd be better.

@jquery/core, what do you think?

@gnarf

This comment has been minimized.

Show comment
Hide comment
@gnarf

gnarf Mar 28, 2015

Member

A JS environment without setTimeout in global is weird place to be....
Though if we find ourselves there, we might want to not explode. Do we already detect/store window somewhere? If so, could we reuse that to find setTimeout?

Member

gnarf commented Mar 28, 2015

A JS environment without setTimeout in global is weird place to be....
Though if we find ourselves there, we might want to not explode. Do we already detect/store window somewhere? If so, could we reuse that to find setTimeout?

@andyoxus

This comment has been minimized.

Show comment
Hide comment
@andyoxus

andyoxus Mar 28, 2015

@gnarf , in the Firefox SDK runtime the global object is a sandbox (of some kind!), and excludes setTimeout (et al.), which can be evil (eval). So, weird, but not crazy.

@mzgol , saving bytes is always good, but I'm now thinking that the Firefox SDK no global setTimeout is a feature, not a bug. As I say, it's a very unusual use case and I'm happy to hack my local jquery.js, but I can imagine other sandboxed run-times in the future. Interesting too that if implemented by checking for window.setTimeout first, you get nice jsdom behaviour. I have no idea how risky that change would be.

andyoxus commented Mar 28, 2015

@gnarf , in the Firefox SDK runtime the global object is a sandbox (of some kind!), and excludes setTimeout (et al.), which can be evil (eval). So, weird, but not crazy.

@mzgol , saving bytes is always good, but I'm now thinking that the Firefox SDK no global setTimeout is a feature, not a bug. As I say, it's a very unusual use case and I'm happy to hack my local jquery.js, but I can imagine other sandboxed run-times in the future. Interesting too that if implemented by checking for window.setTimeout first, you get nice jsdom behaviour. I have no idea how risky that change would be.

@gibson042

This comment has been minimized.

Show comment
Hide comment
@gibson042

gibson042 Mar 28, 2015

Member

If we were to implement this, it would look something like adding a global parameter to the intro.js factory and defining var setTimeout = window.setTimeout || global.setTimeout and similar definitions for other window methods, right? I'm not opposed to exploring that.

Member

gibson042 commented Mar 28, 2015

If we were to implement this, it would look something like adding a global parameter to the intro.js factory and defining var setTimeout = window.setTimeout || global.setTimeout and similar definitions for other window methods, right? I'm not opposed to exploring that.

@gibson042

This comment has been minimized.

Show comment
Hide comment
@gibson042

gibson042 Mar 28, 2015

Member

Or just using the already-available this.

/facepalm

Member

gibson042 commented Mar 28, 2015

Or just using the already-available this.

/facepalm

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Mar 28, 2015

Member

If we were to implement this, it would look something like adding a global parameter to the intro.js factory and defining var setTimeout = window.setTimeout || global.setTimeout and similar definitions for other window methods, right?

It may be hard to sometimes get the global universally; we'd need a special code path for Node and who knows what else (unless we required people to supply it in all non-browser scenarios but that seems hostile).

Or just using the already-available this.

This won't work in Node, this in a module is a module instead of global.

We could create an additional wrapper closure in intro.js and above it define:

var globalSetTimeout = setTimeout;
var globalClearTimeout = clearTimeout;
var globalSetInterval = setInterval;
var globalClearInterval = clearInterval;

and then define a module var/setTimeout.js:

define(function() {
    return window.setTimeout || globalSetTimeout;
});

The same for {set,clear}{Timeout,Interval}.

Member

mgol commented Mar 28, 2015

If we were to implement this, it would look something like adding a global parameter to the intro.js factory and defining var setTimeout = window.setTimeout || global.setTimeout and similar definitions for other window methods, right?

It may be hard to sometimes get the global universally; we'd need a special code path for Node and who knows what else (unless we required people to supply it in all non-browser scenarios but that seems hostile).

Or just using the already-available this.

This won't work in Node, this in a module is a module instead of global.

We could create an additional wrapper closure in intro.js and above it define:

var globalSetTimeout = setTimeout;
var globalClearTimeout = clearTimeout;
var globalSetInterval = setInterval;
var globalClearInterval = clearInterval;

and then define a module var/setTimeout.js:

define(function() {
    return window.setTimeout || globalSetTimeout;
});

The same for {set,clear}{Timeout,Interval}.

@timmywil

This comment has been minimized.

Show comment
Hide comment
@timmywil

timmywil Mar 28, 2015

Member

What about...

define([ "../core" ], function( jQuery ) {
  return 
    // Don't blow up if window isn't available
    // window is only guaranteed defined in the built version of jQuery
    // Prefer window.setTimeout in case of patched version (e.g. jsdom)
    typeof window !== "undefined" && window.setTimeout ||
    // Fall back to global setTimeout if it exists
    typeof setTimeout === "function" ? setTimeout :
    function() {
      jQuery.error("No setTimeout available");  
    };
});
Member

timmywil commented Mar 28, 2015

What about...

define([ "../core" ], function( jQuery ) {
  return 
    // Don't blow up if window isn't available
    // window is only guaranteed defined in the built version of jQuery
    // Prefer window.setTimeout in case of patched version (e.g. jsdom)
    typeof window !== "undefined" && window.setTimeout ||
    // Fall back to global setTimeout if it exists
    typeof setTimeout === "function" ? setTimeout :
    function() {
      jQuery.error("No setTimeout available");  
    };
});
@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Mar 28, 2015

Member

@gnarf

A JS environment without setTimeout in global is weird place to be....

Although it's not a part of ECMAScript so this does happen.

Though if we find ourselves there, we might want to not explode. Do we already detect/store window somewhere? If so, could we reuse that to find setTimeout?

If module/module.exports is defined and the global is not "window-like" (i.e. doesn't contain a document property), window is explicitly passed to the factory by the consumer; the relevant detection code is in intro.js:

jquery/src/intro.js

Lines 25 to 32 in bd9a138

module.exports = global.document ?
factory( global, true ) :
function( w ) {
if ( !w.document ) {
throw new Error( "jQuery requires a window with a document" );
}
return factory( w );
};

@timmywil

What about...

define([ "../core" ], function( jQuery ) {
  return 
    // Don't blow up if window isn't available
    // window is only guaranteed defined in the built version of jQuery
    typeof window !== "undefined" && window.setTimeout ||
    // Fall back to global setTimeout if it exists
    typeof setTimeout === "function" ? setTimeout :
    function() {
      jQuery.error("No setTimeout available");  
    };
});

How would you name this file? If setTimeout.js then our build system will change the return to var setTimeout = which will change the semantics of the detection. My idea avoids this problem. Perhaps checking if window is available at all would need to be added there as well but it's all repetitive so I have a feeling this could be extracted to a helper.

Member

mgol commented Mar 28, 2015

@gnarf

A JS environment without setTimeout in global is weird place to be....

Although it's not a part of ECMAScript so this does happen.

Though if we find ourselves there, we might want to not explode. Do we already detect/store window somewhere? If so, could we reuse that to find setTimeout?

If module/module.exports is defined and the global is not "window-like" (i.e. doesn't contain a document property), window is explicitly passed to the factory by the consumer; the relevant detection code is in intro.js:

jquery/src/intro.js

Lines 25 to 32 in bd9a138

module.exports = global.document ?
factory( global, true ) :
function( w ) {
if ( !w.document ) {
throw new Error( "jQuery requires a window with a document" );
}
return factory( w );
};

@timmywil

What about...

define([ "../core" ], function( jQuery ) {
  return 
    // Don't blow up if window isn't available
    // window is only guaranteed defined in the built version of jQuery
    typeof window !== "undefined" && window.setTimeout ||
    // Fall back to global setTimeout if it exists
    typeof setTimeout === "function" ? setTimeout :
    function() {
      jQuery.error("No setTimeout available");  
    };
});

How would you name this file? If setTimeout.js then our build system will change the return to var setTimeout = which will change the semantics of the detection. My idea avoids this problem. Perhaps checking if window is available at all would need to be added there as well but it's all repetitive so I have a feeling this could be extracted to a helper.

@timmywil

This comment has been minimized.

Show comment
Hide comment
@timmywil

timmywil Mar 28, 2015

Member

@mzgol Yes I'm thinking of a var module.

return to var setTimeout = which will change the semantics of the detection

I'm not catching the problem here. Isn't a var what we want? With a variable named setTimeout at the top of the built file, and every place needing setTimeout referring to the var, there shouldn't be an issue.

Member

timmywil commented Mar 28, 2015

@mzgol Yes I'm thinking of a var module.

return to var setTimeout = which will change the semantics of the detection

I'm not catching the problem here. Isn't a var what we want? With a variable named setTimeout at the top of the built file, and every place needing setTimeout referring to the var, there shouldn't be an issue.

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Mar 28, 2015

Member

@timmywil With your suggestion we'll get this in the built file:

var setTimeout =
    // Don't blow up if window isn't available
    // window is only guaranteed defined in the built version of jQuery
    // Prefer window.setTimeout in case of patched version (e.g. jsdom)
    typeof window !== "undefined" && window.setTimeout ||
    // Fall back to global setTimeout if it exists
    typeof setTimeout === "function" ? setTimeout :
    function() {
      jQuery.error("No setTimeout available");  
    };

The fallback to the global setTimeout is just empty bytes here. If someone passes to the factory a window that doesn't have the setTimeout property because our declaration shadowed the global setTimeout we'll effectively get:

var setTimeout =
    function() {
      jQuery.error("No setTimeout available");  
    };

even though the global setTimeout might exist.

Member

mgol commented Mar 28, 2015

@timmywil With your suggestion we'll get this in the built file:

var setTimeout =
    // Don't blow up if window isn't available
    // window is only guaranteed defined in the built version of jQuery
    // Prefer window.setTimeout in case of patched version (e.g. jsdom)
    typeof window !== "undefined" && window.setTimeout ||
    // Fall back to global setTimeout if it exists
    typeof setTimeout === "function" ? setTimeout :
    function() {
      jQuery.error("No setTimeout available");  
    };

The fallback to the global setTimeout is just empty bytes here. If someone passes to the factory a window that doesn't have the setTimeout property because our declaration shadowed the global setTimeout we'll effectively get:

var setTimeout =
    function() {
      jQuery.error("No setTimeout available");  
    };

even though the global setTimeout might exist.

@timmywil

This comment has been minimized.

Show comment
Hide comment
@timmywil

timmywil Mar 28, 2015

Member

Of course, shadowing...it would be shorter to change the name of the var.

Member

timmywil commented Mar 28, 2015

Of course, shadowing...it would be shorter to change the name of the var.

@gibson042

This comment has been minimized.

Show comment
Hide comment
@gibson042

gibson042 Mar 28, 2015

Member

It may be hard to sometimes get the global universally; we'd need a special code path for Node and who knows what else (unless we required people to supply it in all non-browser scenarios but that seems hostile).

Given that we're not in strict mode, (function() { return this; })() will work everywhere. But as it turns out, we get that for free:

Or just using the already-available this.

This won't work in Node, this in a module is a module instead of global.

That's true (as module.exports) of the outermost context in intro.js (which calls the IIFE), but not true of the IIFE context (which defines module.exports) or the factory context (which defines and returns jQuery). So we could either add setTimeout etc. parameters to factory, or define setTimeout = window.setTimeout || this.setTimeout etc. in factory. I'm inclined towards the former, which won't even break in strict mode. And which is practically indistinguishable from your proposal anyway.

Member

gibson042 commented Mar 28, 2015

It may be hard to sometimes get the global universally; we'd need a special code path for Node and who knows what else (unless we required people to supply it in all non-browser scenarios but that seems hostile).

Given that we're not in strict mode, (function() { return this; })() will work everywhere. But as it turns out, we get that for free:

Or just using the already-available this.

This won't work in Node, this in a module is a module instead of global.

That's true (as module.exports) of the outermost context in intro.js (which calls the IIFE), but not true of the IIFE context (which defines module.exports) or the factory context (which defines and returns jQuery). So we could either add setTimeout etc. parameters to factory, or define setTimeout = window.setTimeout || this.setTimeout etc. in factory. I'm inclined towards the former, which won't even break in strict mode. And which is practically indistinguishable from your proposal anyway.

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Mar 28, 2015

Member

@gibson042

So we could either add setTimeout etc. parameters to factory, or define setTimeout = window.setTimeout || this.setTimeout etc. in factory.

Let me see if I understand you correctly. Do you propose (roughly, modulo comments & size reduction) the following intro.js?

(function( global, factory ) {

    if ( typeof module === "object" && typeof module.exports === "object" ) {
        module.exports = global.document ?
            factory( global,
                w.setTimeout || this.setTimeout,
                w.clearTimeout || this.clearTimeout,
                w.setInterval || this.setInterval,
                w.clearInterval || this.clearInterval,
                true
            ) :
            function( w ) {
                if ( !w.document ) {
                    throw new Error( "jQuery requires a window with a document" );
                }
                return factory( w,
                    w.setTimeout || this.setTimeout,
                    w.clearTimeout || this.clearTimeout,
                    w.setInterval || this.setInterval,
                    w.clearInterval || this.clearInterval
                );
            };
    } else {
        factory( global, global.setTimeout, global.clearTimeout, global.setInterval, global.clearInterval );
    }

// Pass this if window is not defined yet
}(typeof window !== "undefined" ? window : this, function( window, setTimeout, clearTimeout, setInterval, clearInterval, noGlobal ) {

and then, even if we make the whole factory body strict but leave the first IIFE it's still going to work?

Member

mgol commented Mar 28, 2015

@gibson042

So we could either add setTimeout etc. parameters to factory, or define setTimeout = window.setTimeout || this.setTimeout etc. in factory.

Let me see if I understand you correctly. Do you propose (roughly, modulo comments & size reduction) the following intro.js?

(function( global, factory ) {

    if ( typeof module === "object" && typeof module.exports === "object" ) {
        module.exports = global.document ?
            factory( global,
                w.setTimeout || this.setTimeout,
                w.clearTimeout || this.clearTimeout,
                w.setInterval || this.setInterval,
                w.clearInterval || this.clearInterval,
                true
            ) :
            function( w ) {
                if ( !w.document ) {
                    throw new Error( "jQuery requires a window with a document" );
                }
                return factory( w,
                    w.setTimeout || this.setTimeout,
                    w.clearTimeout || this.clearTimeout,
                    w.setInterval || this.setInterval,
                    w.clearInterval || this.clearInterval
                );
            };
    } else {
        factory( global, global.setTimeout, global.clearTimeout, global.setInterval, global.clearInterval );
    }

// Pass this if window is not defined yet
}(typeof window !== "undefined" ? window : this, function( window, setTimeout, clearTimeout, setInterval, clearInterval, noGlobal ) {

and then, even if we make the whole factory body strict but leave the first IIFE it's still going to work?

@timmywil

This comment has been minimized.

Show comment
Hide comment
@timmywil

timmywil Mar 28, 2015

Member

I'd be fine with that, but we are jumping through a lot of hoops. I wonder if it's too much.

Member

timmywil commented Mar 28, 2015

I'd be fine with that, but we are jumping through a lot of hoops. I wonder if it's too much.

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Mar 28, 2015

Member

Of course, shadowing...it would be shorter to change the name of the var.

Yes, but it's going to be harder to contribute, first-time contributors will just assume setTimeout is available, they don't necessarily need to know the magic used to make it available.

Member

mgol commented Mar 28, 2015

Of course, shadowing...it would be shorter to change the name of the var.

Yes, but it's going to be harder to contribute, first-time contributors will just assume setTimeout is available, they don't necessarily need to know the magic used to make it available.

@gibson042

This comment has been minimized.

Show comment
Hide comment
@gibson042

gibson042 Mar 28, 2015

Member

Let me see if I understand you correctly. Do you propose (roughly, modulo comments & size reduction) the following intro.js?

and then, even if we make the whole factory body strict but leave the first IIFE it's still going to work?

Pretty much. And actually, the whole thing will be compatible with strict mode embedding if we avoid this assumptions in the first IIFE (e.g., window.setTimeout || setTimeout instead of window.setTimeout || this.setTimeout).

Member

gibson042 commented Mar 28, 2015

Let me see if I understand you correctly. Do you propose (roughly, modulo comments & size reduction) the following intro.js?

and then, even if we make the whole factory body strict but leave the first IIFE it's still going to work?

Pretty much. And actually, the whole thing will be compatible with strict mode embedding if we avoid this assumptions in the first IIFE (e.g., window.setTimeout || setTimeout instead of window.setTimeout || this.setTimeout).

@markelog

This comment has been minimized.

Show comment
Hide comment
@markelog

markelog Mar 29, 2015

Member

Do we really want to do this? jQuery is DOM-centric, meaning it needs DOM to work correctly, why would you require jQuery if there is no DOM i.e. correct window object?

should be usable for things like Deferreds, etc.

Why? If you want to use Deferred module, use https://github.com/jaubourg/jquery-deferred-for-node, or build your own jquery without core/ready module - grunt custom:-core/ready. If you want to use helpers methods like $.each use lo-dash, underscore, etc, or, yeah, build your own version of jquery with core module only.

Member

markelog commented Mar 29, 2015

Do we really want to do this? jQuery is DOM-centric, meaning it needs DOM to work correctly, why would you require jQuery if there is no DOM i.e. correct window object?

should be usable for things like Deferreds, etc.

Why? If you want to use Deferred module, use https://github.com/jaubourg/jquery-deferred-for-node, or build your own jquery without core/ready module - grunt custom:-core/ready. If you want to use helpers methods like $.each use lo-dash, underscore, etc, or, yeah, build your own version of jquery with core module only.

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Mar 29, 2015

Member

Do we really want to do this? jQuery is DOM-centric, meaning it needs DOM to work correctly, why would you require jQuery if there is no DOM i.e. correct window object?

In an environment where window is not a global we require passing the window to the factory but we still take the timer functions from the global (mainly because they exist in Node and no custom compilation in window-less Node would work if we required setTimeout to lie under window). That's the problem here.

Changing setTimeout to window.setTimeout would fix this issue but would make it harder for Node people.

This inconsistency is the only reason I'm considering fixing this issue as a good idea; if e.g. an environment needs window.getComputedStyle, I'd say: sorry, but we need it. Here we're getting setTimeout passed via window, we just look in a different place.

Member

mgol commented Mar 29, 2015

Do we really want to do this? jQuery is DOM-centric, meaning it needs DOM to work correctly, why would you require jQuery if there is no DOM i.e. correct window object?

In an environment where window is not a global we require passing the window to the factory but we still take the timer functions from the global (mainly because they exist in Node and no custom compilation in window-less Node would work if we required setTimeout to lie under window). That's the problem here.

Changing setTimeout to window.setTimeout would fix this issue but would make it harder for Node people.

This inconsistency is the only reason I'm considering fixing this issue as a good idea; if e.g. an environment needs window.getComputedStyle, I'd say: sorry, but we need it. Here we're getting setTimeout passed via window, we just look in a different place.

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Mar 29, 2015

Member

@markelog Also, see what I wrote before about jsdom. We should be using window.setTimeout when Node is used with jsdom as jsdom patches setTimeout etc. to cancel all timers on window.close. Us using global setTimeout may cause leaks if someone uses window.close. I considered it an edge case, rare enough to not care considering we need global setTimeout for Node anyway. This issue is another such edge case, there may be more.

Member

mgol commented Mar 29, 2015

@markelog Also, see what I wrote before about jsdom. We should be using window.setTimeout when Node is used with jsdom as jsdom patches setTimeout etc. to cancel all timers on window.close. Us using global setTimeout may cause leaks if someone uses window.close. I considered it an edge case, rare enough to not care considering we need global setTimeout for Node anyway. This issue is another such edge case, there may be more.

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Jun 14, 2015

Member

PR: #2391.

Member

mgol commented Jun 14, 2015

PR: #2391.

mgol added a commit to mgol/jquery that referenced this issue Jun 14, 2015

mgol added a commit to mgol/jquery that referenced this issue Jun 14, 2015

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Jun 14, 2015

Member

...and alternative PR: #2392.

Member

mgol commented Jun 14, 2015

...and alternative PR: #2392.

mgol added a commit to mgol/jquery that referenced this issue Jun 15, 2015

mgol added a commit to mgol/jquery that referenced this issue Jun 15, 2015

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Jun 15, 2015

Member

...and another one: #2394.

Member

mgol commented Jun 15, 2015

...and another one: #2394.

@gibson042

This comment has been minimized.

Show comment
Hide comment
@gibson042

gibson042 Jun 15, 2015

Member

Assuing #2394 makes use of setTimeout etc. invalid (which it appears to), I like it best.

Member

gibson042 commented Jun 15, 2015

Assuing #2394 makes use of setTimeout etc. invalid (which it appears to), I like it best.

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Jun 15, 2015

Member

Assuing #2394 makes use of setTimeout etc. invalid (which it appears to

Yup, it does (only in src/, but this is where it matters).

Member

mgol commented Jun 15, 2015

Assuing #2394 makes use of setTimeout etc. invalid (which it appears to

Yup, it does (only in src/, but this is where it matters).

@gibson042

This comment has been minimized.

Show comment
Hide comment
@gibson042

gibson042 Jun 15, 2015

Member

👍

Member

gibson042 commented Jun 15, 2015

👍

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Jun 16, 2015

Member

Fourth PR! #2397.

Member

mgol commented Jun 16, 2015

Fourth PR! #2397.

mgol added a commit to mgol/jquery that referenced this issue Jun 17, 2015

@mgol mgol closed this in #2397 Jun 17, 2015

@dmethvin dmethvin modified the milestones: 1.12/2.2, 3.0.0 Jan 7, 2016

@cssmagic cssmagic referenced this issue May 18, 2016

Open

jQuery #5

@lock lock bot locked as resolved and limited conversation to collaborators Jun 19, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.