doing something simple
browser: cache setTimeout and clearTimeout to isolate from global
Several months ago (June 9th of this year to be exact) a pull request with the above title was opened on node-process, a library that I help maintain which provides a shim for the process object for use in browserify, webpack and other non-node environments. At the end of the day the majority of what it does is simply provide a shim for process.nextTick and what this pull request was doing was making it so that if somebody mucked around with setTimeout (i.e. for testing), the process.nextTick shim would still work.
I thought nothing of it as it seemed like the right approach to take; we use setTimeout to generate the initial async call for process.nextTick (we don’t use it for recursive calls) and we also use clearTimeout in the code so we’d need to save a reference to that as well. The pull was very simple and included a test so it seemed like a win. This turned out to not be the case; within the day of merging it we had 2 issues open.
I intentionally shim setTimeout so I can test streams synchronously
It would seem somebody was making setTimeout be synchronous so that node streams would be synchronous to make writing browser test for code that uses streams be easier to manage.
I couldn’t fix this without reverting the previous change, but in the end we decided not to fix this for a couple reasons.
- The fact we use setTimeout is explicitly not part of our public api, we haven’t always used setTimeout and we might not in the future.
- Given the choice of the code working when people want to stub something unrelated or synchronize async code for convenience, I think not breaking when things are stubbed is a better choice.
- Switching my hat to member of the node streams working group, this does not do what you think it does; streams code often acts in very different ways depending on whether stuff was synchronous or not, so this isn’t actually testing what you think it is.
This ended up being the easy one.
ReferenceError: clearTimeout is not defined
Suddenly we were getting reports of errors thrown because setTimeout and clearTimeout, this is because setTimeout was being referenced outside of functions so there would be a reference to it even if process.nextTick wasn’t called, and some blunders wrap their code globally in ‘use strict’ meaning we get the error (yeah don’t do this with code that you didn’t write). The usual fix for this is to refer to setTimeout (or whatever) as methods of the global object (window, self, global, etc) but if we couldn’t count on setTimeout being there we wouldn’t be able to count on knowing what the global was called or even if ‘this’ would be defined (because it’s executed in a non method function in strict mode) so we make the code somewhat more complex.
TypeError: Invalid Calling Object
You can’t call setTimeout or clearTimeout as a method on any old object (unless you are in node) only as a method of the global object or null/undefined (the spec seams to touch on this obliquely). Now how does this apply here as we are simply calling setTimeout as a normal function, we just happen to refer to it from a different variable, that should matter, right?
It would seem that in IE (and Edge) if your code is executed via eval and setTimeout is renamed then a bug in IE (and Edge) will incorrectly get the wrong context for the the function call. The reason they are even using eval is because webpack uses it for evaluation code in some of their 6 dev modes in order to make source maps better. The fix it to just use .call. This will fix it right?
Short term solution is to avoid using the cached timeout unless somebody has actually changed the global one thus sidestepping the problem the 99% of time people are running code with eval in IE. Except the error shows even when not using eval but only in IE9, it turns out that in IE9 there is a bug where clearTimeout (but not setTimeout) may not be called with null as the context. Why? who knows probably Steve Balmer being spiteful. The fix for this adds some further complications (though it is intentionally broad to catch further corner cases that could pop up) but does feature some my favorite comments I’ve had to write.
Further Down the Rabbit Hole
Do you ever run code which defines globals like setTimeout latter in the execution of the script after your dependencies have run? I don’t because that seems like a recipe for disaster, but somebody does because their code isn’t working. A fix is fairly straight forward and most of the effort on my part was in writing tests.
This brings the total changes needed to do something simple such as cache setTimeout up to something rivaling the rest of the process.nextTick shim.
I mean first off the most obvious thing I’ve learned is that this library is uses way more widely then just browserify, one somewhat assumes that setTimeout is going to be shimmed before node process is but there will always be stranger environments to execute your code. But now it works god damn it and you can shim setTimeout even if you are in an environment that doesn’t have it so you defined it latter and your whole script was evaluated via eval in IE9.
In all honestly these aren’t even the weirdest bugs we’ve had on this project.
Updated 12:30 EDT 9/1/2016:
- Changed link to correct setTimeout spec thanks Domenic Denicola.
- Added link to Edge bug thanks to Nolan Lawson.
Updated 10:42 EDT 9/6/2016
- spelling fixes thanks to Ishmael Smyrnow.