-
Notifications
You must be signed in to change notification settings - Fork 107
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
Fixes ref() & unref() return value on NodeJS #193
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code LGTM although I'm really unsure what behaviour I'd expect.
src/lolex-src.js
Outdated
ref: NOOP, | ||
unref: NOOP | ||
ref: function () { return res; }, | ||
unref: function () { return res; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add .refresh
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the .refresh
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a new API Node recently added in nodejs/node#20298 that landed in 10.2.0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you don't feel like it I'll land and can release when I get back from the Node summit/JSConf next week.
Just saying I would appreciate it if you did it while touching that code - this will land regardless - no pressure.
Already very happy that you decided to contribute to the project :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ohh, I've not known about it. No problem, I'm adding it. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or I need to add some "refresh" logic as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think adding the whole logic is a bit much in this (essentially unrelated PR), but adding a function returning the timeout should be sufficient. Someone else can supply a full PR.NOOP
Purpose (TL;DR) - mandatory
Fix issue #191
According to NodeJS documentation the ref() and unref() return the timer.
https://nodejs.org/dist/latest-v8.x/docs/api/timers.html#timers_timeout_ref
This PR fixes it in lolex. Relevant tests are added.
After fix it prints the correct timer: