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

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

Merged
merged 1 commit into from Jun 10, 2016

Conversation

Projects
None yet
3 participants
@ilgooz
Contributor

ilgooz commented Jun 9, 2016

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:
sinonjs/lolex#66

Show outdated Hide outdated browser.js
@calvinmetcalf

This comment has been minimized.

Show comment
Hide comment
@calvinmetcalf

calvinmetcalf Jun 9, 2016

Collaborator

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

Collaborator

calvinmetcalf commented Jun 9, 2016

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

This comment has been minimized.

Show comment
Hide comment
@ilgooz

ilgooz Jun 9, 2016

Contributor

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

Contributor

ilgooz commented Jun 9, 2016

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

@calvinmetcalf

This comment has been minimized.

Show comment
Hide comment
@calvinmetcalf

calvinmetcalf Jun 9, 2016

Collaborator

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

Collaborator

calvinmetcalf commented Jun 9, 2016

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

This comment has been minimized.

Show comment
Hide comment
@calvinmetcalf

calvinmetcalf Jun 9, 2016

Collaborator

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.

Collaborator

calvinmetcalf commented Jun 9, 2016

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

This comment has been minimized.

Show comment
Hide comment
@ilgooz

ilgooz Jun 9, 2016

Contributor

@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

Contributor

ilgooz commented Jun 9, 2016

@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

Show outdated Hide outdated browser.js
@ilgooz

This comment has been minimized.

Show comment
Hide comment
@ilgooz

ilgooz Jun 9, 2016

Contributor
Contributor

ilgooz commented Jun 9, 2016

@calvinmetcalf

This comment has been minimized.

Show comment
Hide comment
@calvinmetcalf

calvinmetcalf Jun 9, 2016

Collaborator

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();
          });
        });
      });
Collaborator

calvinmetcalf commented Jun 9, 2016

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

This comment has been minimized.

Show comment
Hide comment
@ilgooz

ilgooz Jun 10, 2016

Contributor

right! I've added some tests also for clearTimeout

Contributor

ilgooz commented Jun 10, 2016

right! I've added some tests also for clearTimeout

@calvinmetcalf calvinmetcalf merged commit e481e47 into defunctzombie:master Jun 10, 2016

@calvinmetcalf

This comment has been minimized.

Show comment
Hide comment
@calvinmetcalf

calvinmetcalf Jun 10, 2016

Collaborator

0.11.4

Collaborator

calvinmetcalf commented Jun 10, 2016

0.11.4

@ilgooz

This comment has been minimized.

Show comment
Hide comment
@ilgooz

ilgooz Jun 10, 2016

Contributor

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()

Contributor

ilgooz commented Jun 10, 2016

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

This comment has been minimized.

Show comment
Hide comment
@calvinmetcalf

calvinmetcalf Jun 10, 2016

Collaborator

oh i see fixed 53fdf64

Collaborator

calvinmetcalf commented Jun 10, 2016

oh i see fixed 53fdf64

@leidegre

This comment has been minimized.

Show comment
Hide comment
@leidegre

leidegre Jun 28, 2016

This commit actually breaks IE11 (possibly not under all circumstances). I get a Invalid calling object error when invoking cachedSetTimeout. This does only appear to be an issue with IE11 and it appears to be related to how calling context for globals are captured. I'm leaving this here for other's to find in case they run into a similar issue.

This broke bluebird as a browser polyfill for promises in IE11.

leidegre commented on e481e47 Jun 28, 2016

This commit actually breaks IE11 (possibly not under all circumstances). I get a Invalid calling object error when invoking cachedSetTimeout. This does only appear to be an issue with IE11 and it appears to be related to how calling context for globals are captured. I'm leaving this here for other's to find in case they run into a similar issue.

This broke bluebird as a browser polyfill for promises in IE11.

This comment has been minimized.

Show comment
Hide comment
@leidegre

leidegre Jun 28, 2016

I should also add that simply using the previous version works just fine. So, to fix this

npm install process@0.11.1

Make sure you add that exact version to your package.json file.

leidegre replied Jun 28, 2016

I should also add that simply using the previous version works just fine. So, to fix this

npm install process@0.11.1

Make sure you add that exact version to your package.json file.

This comment has been minimized.

Show comment
Hide comment
@calvinmetcalf

calvinmetcalf Jun 28, 2016

Collaborator

can you open an issue with how you were able to trigger this?

Collaborator

calvinmetcalf replied Jun 28, 2016

can you open an issue with how you were able to trigger this?

This comment has been minimized.

Show comment
Hide comment
@leidegre

leidegre replied Jun 28, 2016

@leidegre

This comment has been minimized.

Show comment
Hide comment
@leidegre

leidegre Jun 28, 2016

This broke bluebird as a browser polyfill for promises in IE11. See longer comment further down on this commit.

leidegre commented on browser.js in e481e47 Jun 28, 2016

This broke bluebird as a browser polyfill for promises in IE11. See longer comment further down on this commit.

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