-
Notifications
You must be signed in to change notification settings - Fork 313
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
Increase compatiblity with faking time in test tools #15
Increase compatiblity with faking time in test tools #15
Conversation
d56daee
to
4876183
Compare
I feel like sinon.useFakeTimers should affect this library. Why do you feel it shouldn't? I imagine there are people using fakeTimers with this library currently. |
I ran into issues where my time dependent tests that use Here's my understanding of how promise-polyfill is using We define an If that is correct, then this PR ensures that promise is kept (accidental pun) even when the global�
I would imagine so as well. Perhaps best bump the major version, if this PR is merged. |
Hi @taylorhakes, thanks for your prompt reply! I ran into the same issue as (well, together with) @mroderick. I tried to exemplify the reasoning behind the shadowing in this jsbin: http://jsbin.com/metoma/edit?html,js,output I pasted a near-verbatim version of I hope that was clear enough. We had to make the change in this PR in order for our tests (somewhat similar to the test in the bin) to pass. Do you think it makes sense? |
Thanks @mroderick and @codazzo for the detailed explanations. I completely understand your reasoning now. I plan to move forward with this merge, but I would like to let this sit open for a couple more days. I want to make sure I am not forgetting something. If others have any objections, it will give them time to speak up. I will plan to make this change for v3. |
This change can be implemented in one line I believe. setImmediate is already stored locally on the asap line. // Store setTimeout to not be affected by mocking
var setTimeout = setTimeout; Do you mind making that change? Also, please format it with tabs instead of spaces. |
4876183
to
235ab1f
Compare
Not at all. I've pushed changes and the build is green :)
My apologies, I am so used to having an |
Do we need to reference var setTimeout = setTimeout; We then also don't need the change at the bottom with global. The global line at the bottom won't work in browsers. I should probably make the build run in browsers to catch those types of errors. Please add the a comment above the setTimeout line. Something like // Store setTimeout to not be affected by mocking to make sure we remember why it is being done. |
By caching reference to the global setImmediate and setTimeout we can ensure that promise-polyfill still works as excpected when sinon.useFakeTimers() replaces them with fake versions.
235ab1f
to
719c224
Compare
Updated as requested |
Increase compatiblity with faking time in test tools
Perfect. I will release a new version to NPM tonight |
Thanks! |
Thanks! 🎉 |
Created version 3.0.0. https://github.com/taylorhakes/promise-polyfill/releases/tag/3.0.0 |
By caching reference to the global setImmediate and setTimeout we can ensure
that promise-polyfill still works as excpected when using fake timers in unit test scenarios.
See