New version breakage

[yoshicarroll]

Hello,

I'm not entirely sure if this is the right place to document this problem, but the latest version of process is breaking for us on the lines below.

// cached from whatever global is present so that test runners that stub it don't break things.
var cachedSetTimeout = setTimeout;
var cachedClearTimeout = clearTimeout;

Webpack includes process into the bundles it creates, and we execute the bundles on the client and on the server for rendering React components. For the server rendering there is no setTimeout, so the new code fails.

Do you think the problem is with process or with webpack?

Thanks

[calvinmetcalf]

setTimeout is 100% available in node on the server

[calvinmetcalf]

so some more details about how you are generating the bundle and how you are running it would be appreciated

[rdy]

This is causing a problem for me as well, primarily in tests where we mock out setTimout and clearTimeout. With the new change to cache these functions to isolate them from global they are instead bound to unexpected versions of setTimeout. In our usage we want these functions to be replaced by our mocks instead of remaining the global versions instead.

it is possible for us to force the cached versions to be the same mocked versions by either making sure we mock before requiring the file the the bundle or replacing the implementation to rely on the global setTimeout and clearTimeout.

I'm not entirely certain why this change was made to begin with? Is there a strong need to make sure the original functions are isolated? I might suggest making this a breaking change to the module due to the change in this behavior so other people are less surprised by it.

[ilgooz]

@rdy I don't understand the use case of mocking the setTimeouts inside of nextTick. maybe you should mock nextTick itself? using setTimeout is a hack to simulate functionality of nextTick and programs should not rely on how nextTick achieves it's goal. the change is actually made to avoid setTimeout to getting stubbed by testing helpers at runtime to avoid code execution to hang out.

[calvinmetcalf]

So this was done to prevent us from seeing mocked out versions of set timeout as that was breaking a lot of stuff including several test runners. What exactly were you doing were you needed this module to use a different version of setTimeout.

[ilgooz]

for detailed information about the change please see sinonjs/fake-timers#66

[rdy]

our use case is in using node streams in the browser but our tests. we specifically want to control the stream using a mocked setTimeout so we can synchronously test the results of interactions with the stream object.

I haven't really had the use case of needing asynchronous behavior in the test environment. FYI we are using jasmine.

Here is an example of what we're doing

      it('creates a subscription with the correct config', () => {
        subject.subscribe({appGuid, startTime, tickSize, type, metricsType}, {jarviceStream, accessToken});
        jasmine.clock().tick(1);
        expect(subject.subscribeTo).toHaveBeenCalledWith({endpoint: toSubscriptionUrl({streamId, type, appGuid, metricsType, tickSize, startTime})}, jasmine.objectContaining({accessToken}));
      });

In order to preserve the original behavior our streams would have to run asynchronously which is something we're trying to avoid. I understand the use case in sinon but i think that is different.

[rdy]

This is probably due to the fact our streams are relying on using nextTick simulated with setTimeout. I believe that the change made it much easier to deal with asynchronous testing but it really breaks synchronous style testing with mocks against the timers.

The problem is that the node streams are polyfilled by relying on nextTick which uses setTimeout. We could add a mock on that as well but this definitely is breaking behavior regardless of what the "correct" behavior should be.

When adding global mocks to setTimeout and clearTimeout for testing purposes, I expect them to use the mocked versions instead of cached versions, what would be the point of mocking setTimeout globally if other objects didn't respect that mock?

[calvinmetcalf]

the fact this module uses setTimeout has not always been the case and may
not always be the case so you shouldn't consider it part of the public api.

Switching hats to me being a member of the node streams working group.
Making process.nextTick sync is not a good way to test steams as it will
alter their behavior in unforseen ways. Seriously I highly doubt it's as
robust a test as you think it is

On Fri, Jun 10, 2016, 4:51 PM Ryan Dy [email protected] wrote:

This is probably due to the fact our streams are relying on using nextTick
simulated with setTimeout. I believe that the change made it much easier to
deal with asynchronous testing but it really breaks synchronous style
testing with mocks against the timers.

—
You are receiving this because you commented.

Reply to this email directly, view it on GitHub
#60 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/ABE4nx9RgW9peQDDKJrXEc2p5_m3bjaYks5qKc5BgaJpZM4IzNnh
.

[yoshicarroll]

@calvinmetcalf We're not loading the bundle in node, just the V8 engine. From .net.

[rdy]

We can lock down to the older version if the current cached behavior is the preferred implementation.

I'm in favor of not changing the semantics for the test environments, if I'm mocking setTimeout and clearTimeout globally I would expect anything that uses those calls to be also mocked, not the other way around. That seems to be more typical of things that rely on these globally available functions.

[calvinmetcalf]

@rdy I'd suggest mocking before requiring and locking down both steams and process

@yoshicarroll are you wrapping it in strictmode as well?

[ilgooz]

@rdy following may help with the new situation. you can initialize it with before hook and restore with an after hook and use tick() to execute the next cb. I haven't tested the code tho. but webpack replaces process where it sees, with a reference to this package in build time to shim. I don't know if there is any config to disable it. if you can disable process being automatically replaced then you can create a global var process = require('process') at your highest level component so following code can manipulate process inside other modules.

function NextTicker(){
  this.queue = []
  this.originalNextTick = process.nextTick
  process.nextTick = function(cb){
    this.queue.push(cb)
  }
}
NextTicker.prototype.tick = function(){
  var cb = this.queue.shift()
  cb()
}
NextTicker.prototype.restore = function(){
  this.queue.forEach(function(cb){
   cb()
  })
  this.queue = []
  process.nextTick = this.originalNextTick
}

var ticker = new NewTicker() // before hook
ticker.tick() // tick as many for your assertions
ticker.tick()
ticker.restore() // after hook

[rdy]

Yea I thought about doing this as well @ilgooz.

I was able to work around the whole problem by locking down to 0.11.3 but I'll probably add the code to make sure the replacement of the setTimeout occurs before requiring process for the first time.

I should be able to completely replace the module with webpack to work around the problem of it being shimmed but that is almost the same as locking down to the older version for now. I might just end up mocking the nextTick method in process inside of jasmine and replace it with a version that does not rely on the cached timeouts.

Thanks for the advice.

[yoshicarroll]

@calvinmetcalf Yeah. Default behavior with Babel.

[calvinmetcalf]

Dangerous to do stict mode on code that might not be strict mode compatible
in general, double so when not supplying globals that that some libraries
expect. We can likely do a check of some sort to make sure it doesn't throw
an early error, but you'll get another error if you have anything relying
on process.nextTick

On Fri, Jun 10, 2016, 7:13 PM Yoshi Carroll [email protected]
wrote:

@calvinmetcalf https://github.com/calvinmetcalf Yeah. Default behavior
with Babel.

—
You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
#60 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/ABE4n7VGoGpCA9iLy0Y3OyT_Fe4oecrDks5qKe-ugaJpZM4IzNnh
.

[calvinmetcalf]

@yoshicarroll #61 should fix your issue

@rdy I'm sorry your test broke but to be honest if it wasn't this than it would have been something in streams which makes some assumptions about process.nextTick being async. If you need help testing streams async open an issue at readable-streams and I'll be happy to help.

relevant XKCD