Skip to content
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

Set timeout promise support #227

Closed
wants to merge 2 commits into from
Closed

Conversation

benjamingr
Copy link
Member

Purpose (TL;DR) - mandatory

Fixes #223 by supporting util.promisify on fake setTimeout

Background (Problem in detail) - optional

By the way, I started a new job at @testimio (actually doing test related stuff) - can I change the email in AUTHORS?

@benjamingr
Copy link
Member Author

cc @orlangure this unblocks the "how-to guide" PR

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to be failing CI?

Should this get a mention in the docs?

@@ -568,6 +570,13 @@ function withGlobal(_global) {
delay: timeout
});
};
if (symbolUtilPromisifyCustom) {
clock.setTimeout[symbolUtilPromisifyCustom] = function (delay) {
return new _global.Promise(function (resolve) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you store a reference to Promise at initialization time? Would help in Jest in the cases where people mess with globals

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, can you elaborate on that?

Copy link
Member

@SimenB SimenB Mar 14, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

People have tests that do

global.Promise = undefined

inside of their test. It would be great to be resilient to that by grabbing a copy of the original Promise early. Use case being to test how their code behaves without promises available

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SimenB You're basically just saying that lolex should do something like this, right?

// needed not to break in case people null out the `Promise` property in user-code
var originalPromise = _global.Promise;

And then reference that when using Promises internally.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes 🙂

@mroderick mroderick changed the title Set timeout promise suppot Set timeout promise support Nov 17, 2018
@orlangure
Copy link

Guys, any news here?
@benjamingr can you take a look at failing CI?

@fatso83
Copy link
Contributor

fatso83 commented Mar 14, 2019

@benjamingr No idea why it stalled, but regarding the point on AUTHORS, the answer is no.

That is not a "No, you are not allowed to", but "No, it's pointless, as it will be overwritten by a script".

@benjamingr
Copy link
Member Author

I'm not sure why this stalled either, seems important.

@stale
Copy link

stale bot commented May 13, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label May 13, 2019
@ehmicky
Copy link

ehmicky commented May 13, 2019

Commenting to remove the stale label.

@stale stale bot removed the stale label May 13, 2019
@SimenB SimenB added the pinned label May 13, 2019
this.clock.tick(10);

assert(lolex.evalCalled);
assert(evalCalled);
delete global.evalFn;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delete this before the assertion so that it's guaranteed to get called. If the assertion above fails, this won't be cleaned up.

@fatso83
Copy link
Contributor

fatso83 commented Jun 27, 2019

This has been inactive for 7 months, so thought I'd try to understand what's blocking progress. Maybe we are seeing a social deadlock? 🤓

@benjamingr Are you waiting for someone to do something? Because I think this has been reviewed, everyone thinks its good, but it just needs to address a couple of very minor issues - and the failing CI build, of course.

I can't see anything else blocking this, but I've been blind to the most obvious stuff before, so ...

@benjamingr
Copy link
Member Author

@benjamingr Are you waiting for someone to do something? Because I think this has been reviewed, everyone thinks its good, but it just needs to address a couple of very minor issues

Yeah this is on me, I'm setting up an appointment to fix this in my next slot in my calendar so this doesn't get missed again. Sorry, it keeps slipping my mind since I never run into this issue with lolex myself.

@SimenB
Copy link
Member

SimenB commented Nov 11, 2019

@benjamingr ping 😀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Promisified (with util.promisify) setTimeout does not work
6 participants