timer: ref/unref return self

[sam-github]

Most calls to ref() and unref() are chainable, timers should be
chainable, too.

Typical use:

var to = setTimeout(ontimeout, 123).unref();

/to @trevnorris This doesn't appear to me to have performance implications, look OK to you?

[cjihrig]

LGTM

[Fishrock123]

Seems fine to me?

LGTM. CI for good measure: https://ci.nodejs.org/job/node-test-pull-request/316/

[sam-github]

Thanks, I'll merge after CI is green.

[sam-github]

And after @trevnorris chimes in.

[silverwind]

I initially planned on doing this in #1768 (comment), but the locked API intimitated me back then. I wouldn't expect breakage from this, thought, so +1 from me.

[silverwind]

BTW: Can we either document close or make it _close?

[sam-github]

Probably can't rename it, maybe could document it, though I don't know if it has a use case. Isn't the timer API inherited from browsers?

[silverwind]

Node's Timer API is pretty different from what browsers use. Browsers return numerical IDs while we return Timeout objects. I'd say if there's no public use case, we should move it to _close.

In the long run, I'd like to see us getting closer to the standardized WindowTimers which means returning numerical values, and possibly introduce helper methods for added functionality (Timer#ref for example).

[trevnorris]

@silverwind Agreed that this doesn't voilate the conceptual "Locked" status.

LGTM

[sam-github]

Landed in 94e663a

[silverwind]

Can we give PRs like this a bit more time so collaborators get a chance to voice their opinion? We should at least try to leave roughly 48 hours before landing important changes as stated in the collaborator guide. Just two hours is definitely not enough imho, even with the amount of LGTM's this has.

Also, my question about close looks to have been ignored so we've now extended public non-underscore API without documenting it, which is pretty bad on its own, imho.

[rvagg]

thanks for that @silverwind, 2 hours between PR and landing is a bit extreme, remember that we're dealing with devs around the world with very different schedules from each other and we need to provide opportunity for input to make the most of the broad collaborator group

[sam-github]

I was the under the impression that trivial changes regularly land in under 48 hours, but I will wait longer next time.

@silverwind I didn't ignore your comment, I responded to it, but your comment was also wholly unrelated to my PR.

You suggest documenting a previously undocumented API: maybe a good idea, maybe not, but not related to chainability of ref/unref.

You also suggested renaming that API: again, maybe a good idea, maybe not, but also unrelated to chaining of ref/unref.

Using this PR to have a conversation about some other thing you'd like to change in code nearby is fine by me, but it didn't appear to me to be have any impact on the progress of this PR.

Am I misunderstanding your comment?

[sam-github]

Also, @silverwind, you +1ed this, another reason I thought your comment was a conversation, not an issue needing resolution.

[sam-github]

@rvagg there are 4 doc PRs (not including the two I did) and one change to Buffer that have landed within the last day with less than a day since PR, unless I misread https://github.com/nodejs/node/pulls

How hard is that 48 hour guideline? Does it apply to everything, even docs?

And is it 48 hours since initial PR, or 48 hours since last comment?

[silverwind]

Well, you do not mention the change to close anywhere in the commit title, commit message or comments, which makes me think you're trying to hide or downplay the change. We often find areas to improve while discussing PRs which are unrelated to the PR itself (and that is a good thing!), but I think we should resolve any open discussions before landing the PR.

[rvagg]

The fact that this has to be labelled semver-minor is one reason this can't be called trivial. It's hard to define what trivial means because you can do major things in a small number of lines of code. Doc changes are usually more towards the trivial end than code changes but they should still usually be given time. An easy catch for "trivial" would be spelling fixes in docs or comments, they're usually unlikely to be controversial unless it's a matter of US English vs UK English or ...

One reason for delaying merging and allowing time for feedback is that there's a ton of collective knowledge about Node, how it's used, its history and the details of various discussions that may have happened. In this case, there may be reasons that we've decide to not make these APIs chainable, perhaps there's a bigger plan to do something else with these APIs that are ruled out by returning this, perhaps there's a performance reason, perhaps there's something related to how browsers implement these and an explicit decision not to go forward.

Also, the collaborator team brings such a broad range of skills an expertise to the project and we need to maximise our use of them—some people are good at reviewing JavaScript, some people understand V8 better, some are obsessed with performance, etc. so a big reason for allowing time is to make sure we make full use of the entire team.

I don't mean to pick on you here @sam-github, there's been plenty of examples of unnecessarily quick merges, I think this is a topic that we're going to be continually revisiting and calibrating.

Lastly, unless a change is required to get a release out, there's no reason to rush, let's be more slow and intentional about this and be sure to lean on each other.

[sam-github]

I'm not "hiding" anything, the commit message is already 3 times longer than the 6 word code change, it didn't seem noteworthy.

I changed close because it was there, and not changing it would lead to inconsistent code.

@silverwind you would prefer close not to return this?

[silverwind]

@silverwind you would prefer close not to return this?

No, that's fine, even thought I'm not sure when you'd want to chain off close().

More importantly, I noticed the setTimeout(ontimeout, 123).unref() case you mentioned already worked before this commit. Why would you chain off ref and unref when the Timeout object is only obtainable throught setTimeout or setInterval earlier in the chain?

[sam-github]

I think you are neglecting to consider the assignment part of the example, its not optional, you can't call clearTimeout() on undefined:

/tmp % node -v
v4.0.0
/tmp % node
> t = setTimeout(console.log, 10).unref()
undefined
> t
undefined

With this change, t is not undefined.