dominictarr / through

simple way to create a ReadableWritable stream that works
Other
669 stars 64 forks source link

Should keep internal references to globals #43

Closed fatso83 closed 8 years ago

fatso83 commented 8 years ago

We got a bug report our way in sinonjs/lolex#66, which basically boils down to that Sinon stubs global timing functions and external library and framework code (such as tape and through) gets hits because they don't cache references internally. Mocha solves this issue by saving references to all global timing related functions, such as setTimeout, setInterval, nextTick, ..., so that later modifications will not interfere with its internal use of them.

This would be the easiest fix.

This line is problematic.

Related to substack/tape#292

ilgooz commented 8 years ago

@fatso83 thank you for tracking down this issue! I think we don't need any changes on through since it uses only process.nextTick. defunctzombie/node-process now supports cached timeouts. browserify and webpack bundles will work smoothly where a setTimeout used in nextTick from now on. process object will be automatically replaced by webpack with defunctzombie/node-process.

fatso83 commented 8 years ago

Well, keeping a reference on globals would ensure that other processes could not mess with your internal logic, so it's just a good measure in any case.

dominictarr commented 8 years ago

I have to say I'm against changing through because sinon is messing with things. sinon should instead do what it needs to do without breaking anything. It's rather ironic that a testing library creates new, hard to debug problems, when the purpose of testing is to make development easier.

fatso83 commented 8 years ago

It's rather unavoidable to alter global state since that is the actual purpose of the calls to stub out global timing functions :-) But it's no biggie, and it seems ilgooz has found a way around this, so nvrmnd.