defunctzombie / node-process

process information for node.js and browsers
MIT License
122 stars 62 forks source link

browser: cache setTimeout and clearTimeout to isolate from global #58

Closed ilgooz closed 8 years ago

ilgooz commented 8 years ago

tl;dr: sinon stubs time related global objects for unit testing and this breaks all packages depended on process.nextTick. caching setTimeout and clearTimeout at bootstrap protects against mutations.

related issue: https://github.com/sinonjs/lolex/issues/66

calvinmetcalf commented 8 years ago

I think the update can be simplified to just 2 lines that cache them with the same name, a test would be very helpful as well

ilgooz commented 8 years ago

@calvinmetcalf of course. you mean window as global with var setTimeout = window.setTimeout syntax? otherwise global would only work with nodejs.

calvinmetcalf commented 8 years ago

browserify/webpack sets global to be window in the browser or self in a web worker so it's a shorthand to the global object in any of those envs

calvinmetcalf commented 8 years ago

actually though it wouldn't shock me if this was used in some env that didn't give global, so your way is probably good.

a comment explaining this is probably a good idea.

ilgooz commented 8 years ago

@calvinmetcalf js world does not shock me anymore :). is it good this way? I couldn't come up with a test case though any suggestions welcome

ilgooz commented 8 years ago

@calvinmetcalf :+1:

calvinmetcalf commented 8 years ago

test for you

describe('rename globals', function (t) {
        it('throws an eroror', function (done){
          var oldTimeout = setTimeout;
          setTimeout = function () {
            setTimeout = oldTimeout;
            assert.ok(false);
            done();
          }
          ourProcess.nextTick(function () {
            assert.ok(true);
            setTimeout = oldTimeout;
            done();
          });
        });
      });
ilgooz commented 8 years ago

right! I've added some tests also for clearTimeout

calvinmetcalf commented 8 years ago

0.11.4

ilgooz commented 8 years ago

thanks! @calvinmetcalf fyi, setting clearTimeout to original one inside nextTick for cleanup has a side effect that makes tests to do not complain when clearTimeout replaced with original one because of execution order inside drainQueue()

calvinmetcalf commented 8 years ago

oh i see fixed https://github.com/defunctzombie/node-process/commit/53fdf64e07ae76c74344f4974569ddfcd8262736