doc: general improvements to timers.md #6937

jasnell · 2016-05-23T21:13:33Z

Checklist

documentation is changed or added
the commit message follows commit guidelines

Affected core subsystem(s)

doc (timers)

Description of change

General improvements to timers.md copy

@nodejs/documentation

eljefedelrodeodeljefe · 2016-05-23T21:28:52Z

doc/api/timers.md

-All of the timer functions are globals.  You do not need to `require()`
-this module in order to use them.
+The `timer` module exposes a global API for scheduling callback functions to
+execute at some future period of time. Because the timer functions are globals,


execute is "one-off", period would rather indicate a sequence of time, no? Anyhow this sounds a little bulky.

eljefedelrodeodeljefe · 2016-05-23T21:29:43Z

Generally LGTM. Good work.

addaleax · 2016-05-23T22:57:30Z

doc/api/timers.md


+[the Node.js Event Loop]: ../topics/the-event-loop-timers-and-nexttick.html


Are you sure this works?

Not yet. Will be double checking the links before landing.
On May 23, 2016 3:57 PM, "Anna Henningsen" notifications@github.com wrote:

In doc/api/timers.md
#6937 (comment):

+[the Node.js Event Loop]: ../topics/the-event-loop-timers-and-nexttick.html

Are you sure this works?

—
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
https://github.com/nodejs/node/pull/6937/files/a8c72718f4500daa613cfe84416f19e8664f6378#r64303631

I don't think so

mscdex · 2016-05-24T06:57:22Z

/cc @Fishrock123

Fishrock123 · 2016-05-24T14:04:40Z

doc/api/timers.md

+
+By default, when a timer is scheduled using either `setTimeout()` or
+`setInterval()`, the Node.js event loop will continue running as long as the
+timer is active. Each of the opaque timer objects returned by these functions


"opaque" should be something along the lines of "internally tracked" or something

i.e. no access to a _handle

Fishrock123 · 2016-05-24T14:06:31Z

Should be blocked by #6206 or that should be incorporated, but I'd like to preserve @bengl's contribution if possible.

It should have already landed I think but I haven't gotten around to it.

jasnell · 2016-05-24T15:15:30Z

@bengl ... looking at #6206, how about we combine efforts into a single PR? I can port your edits on top of mine (keeping it as a separate commit with your name attached).

bengl · 2016-05-27T21:40:03Z

@jasnell Yeah sure. You want me to PR against your branch or you got it?

jasnell · 2016-05-27T21:46:30Z

Either way works!
On May 27, 2016 2:40 PM, "Bryan English" notifications@github.com wrote:

@jasnell https://github.com/jasnell Yeah sure. You want me to PR
against your branch or you got it?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#6937 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AAa2eZi4KT-7AqEqhXPOQwPXviks2z5Jks5qF2S6gaJpZM4Ik4sD
.

bengl · 2016-05-27T22:27:27Z

@jasnell alright I made a PR against your branch with roughly the same changes (it was easier to do a completely commit than to rebase).

jasnell · 2016-05-28T00:03:56Z

Awesome, thank you. Squashed my commits and pulled yours in.
@Fishrock123 ... LGTY?

jasnell · 2016-05-28T00:09:47Z

@bengl ... made a few additional edits on top .. PTAL

bengl · 2016-05-28T01:35:41Z

LGTM

benjamingr · 2016-05-28T07:27:10Z

LGTM

Fishrock123 · 2016-05-28T14:52:34Z

doc/api/timers.md

-request the timer hold the program open. If the timer is already `ref`d calling
-`ref` again will have no effect.
+When called, requests that the Node.js event loop *not* exit so long as the
+timer is active. Calling `timer.ref()` multiple times will have no effect.


Has no effect unless the Timeout was previously unrefed.

Also this does not restore overhead lost by calling unref() -- once you unref it, we'd have to do a linear walk to re-insert it.

Neither of these comments seem to be necessary to add to the user documentation in this PR.

jasnell · 2016-06-02T23:16:34Z

Updated. PTAL

jasnell · 2016-06-03T19:47:28Z

If there are no further comments, I will land on Monday

Fishrock123 · 2016-06-03T20:01:46Z

doc/api/timers.md

-[`clearImmediate`][]. Additional optional arguments may be passed to the
-callback.
+When called, the `Timeout` object (while active) will not require the Node.js
+event loop to remain active. Calling `timout.unref()` multiple times will have


Not incorrect, but I think this is still poorly worded.

When called, the Timeout will not keep the event loop open. When Node.js would exit due to inactivity, this timer will longer be considered as activity, and the program may stop before it calls it's callback.

Maybe that would be better? I agree the wording is hard without having a strong idea of the event loop and handle refedness.

I'm not sure that's any better. Rather than hold this up while we get the wording perfect on this, I'd recommend landing and iterating on it separately in subsequent PRs

Hmmm, perhaps, but it should probably at least indicate it has an effect on when the process exits. Perhaps the original wording is the best compromise for now then?

Fishrock123 · 2016-06-03T20:03:22Z

LGTM minus nits (particularly the last one)

Fishrock123 · 2016-06-15T20:06:19Z

ping @jasnell

jasnell · 2016-06-15T20:22:27Z

Will be back on this after I'm back from vacation
On Jun 15, 2016 1:06 PM, "Jeremiah Senkpiel" notifications@github.com
wrote:

ping @jasnell https://github.com/jasnell

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#6937 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AAa2eaKRtSzCcz4qv6BkCJ3KMN9bk3oUks5qMFtCgaJpZM4Ik4sD
.

jasnell · 2016-06-21T17:30:59Z

Updated one final time to address nit

Fishrock123 · 2016-06-27T12:06:12Z

doc/api/globals.md

@@ -208,7 +208,7 @@ but rather than loading the module, just return the resolved filename.
 [native addons]: addons.html
 [timers]: timers.html
 [`clearImmediate`]: timers.html#timers_clearimmediate_immediateobject
-[`clearInterval`]: timers.html#timers_clearinterval_intervalobject
+[`clearInterval`]: timers.html#timers_clearinterval_immediateobject


I think this link is incorrect?

Fishrock123 · 2016-06-27T12:11:38Z

LGTM, might want to make sure the link is right though. The other bits can always be addressed in a new PR

jasnell · 2016-06-27T16:04:40Z

Updated to address the final round of nits

Fishrock123 · 2016-06-28T10:00:38Z

Seems good, let's land and unblock #6956

@bengl

Overall improvements to timers.md documentation, Includes squashed commit from @bengl: doc: add timer classes The timers returned by `setTimeout` and friends are actually instances of `Timeout` and `Immediate`. Documenting them as such, so that the `ref` and `unref` methods can be identified as methods on `Timeout` objects. Sparked by discussion in nodejs#5792

@bengl

Overall improvements to timers.md documentation, Includes squashed commit from @bengl: doc: add timer classes The timers returned by `setTimeout` and friends are actually instances of `Timeout` and `Immediate`. Documenting them as such, so that the `ref` and `unref` methods can be identified as methods on `Timeout` objects. Sparked by discussion in #5792 PR-URL: #6937 Reviewed-By: Robert Jefe Lindstaedt <robert.lindstaedt@gmail.com> Reviewed-By: Bryan English <bryan@bryanenglish.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>

jasnell · 2016-06-28T14:40:45Z

Landed in 86e07b7

@bengl

Overall improvements to timers.md documentation, Includes squashed commit from @bengl: doc: add timer classes The timers returned by `setTimeout` and friends are actually instances of `Timeout` and `Immediate`. Documenting them as such, so that the `ref` and `unref` methods can be identified as methods on `Timeout` objects. Sparked by discussion in #5792 PR-URL: #6937 Reviewed-By: Robert Jefe Lindstaedt <robert.lindstaedt@gmail.com> Reviewed-By: Bryan English <bryan@bryanenglish.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>

jasnell added doc Issues and PRs related to the documentations. timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. labels May 23, 2016

eljefedelrodeodeljefe reviewed May 23, 2016
View reviewed changes

addaleax reviewed May 23, 2016
View reviewed changes

Fishrock123 reviewed May 24, 2016
View reviewed changes

Fishrock123 mentioned this pull request May 24, 2016

timers: warn on overflowed timeout duration #6956

Closed

3 tasks

bengl mentioned this pull request May 27, 2016

doc: add timer classes jasnell/node#9

Closed

jasnell force-pushed the doc-timers-copy branch from f39fc78 to 3b23032 Compare May 28, 2016 00:03

jasnell force-pushed the doc-timers-copy branch from dbdd13a to e0ee2df Compare May 28, 2016 01:36

Fishrock123 reviewed May 28, 2016
View reviewed changes

Fishrock123 reviewed Jun 3, 2016
View reviewed changes

jasnell mentioned this pull request Jun 6, 2016

doc: more detail on when timers are executed #7166

Closed

2 tasks

bengl mentioned this pull request Jun 6, 2016

doc: add timer classes #6206

Closed

2 tasks

Fishrock123 removed their assignment Jun 15, 2016

jasnell force-pushed the doc-timers-copy branch from b707c46 to 5036973 Compare June 21, 2016 17:30

Fishrock123 reviewed Jun 27, 2016
View reviewed changes

jasnell force-pushed the doc-timers-copy branch from 5036973 to fb05064 Compare June 27, 2016 16:04

jasnell force-pushed the doc-timers-copy branch from fb05064 to 4091f25 Compare June 28, 2016 14:28

jasnell closed this Jun 28, 2016

Fishrock123 mentioned this pull request Jul 5, 2016

Propose v6.3.0 (v2) #7550

Merged

MylesBorins mentioned this pull request Jul 11, 2016

Audit commits not found on v4.x-staging #7661

Closed

MylesBorins added the dont-land-on-v4.x label Jul 11, 2016

blacksun1 mentioned this pull request Jan 17, 2017

observed setImmediate and setTimeout execution order contradicts documentation #7145

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

doc: general improvements to timers.md #6937

doc: general improvements to timers.md #6937

jasnell commented May 23, 2016

eljefedelrodeodeljefe May 23, 2016

eljefedelrodeodeljefe commented May 23, 2016

addaleax May 23, 2016

jasnell May 23, 2016

Fishrock123 May 24, 2016

mscdex commented May 24, 2016

Fishrock123 May 24, 2016 •

edited

Loading

Fishrock123 commented May 24, 2016

jasnell commented May 24, 2016

bengl commented May 27, 2016

jasnell commented May 27, 2016

bengl commented May 27, 2016

jasnell commented May 28, 2016

jasnell commented May 28, 2016

bengl commented May 28, 2016

benjamingr commented May 28, 2016

Fishrock123 May 28, 2016

Fishrock123 May 28, 2016

jasnell May 31, 2016

jasnell commented Jun 2, 2016

jasnell commented Jun 3, 2016

Fishrock123 Jun 3, 2016

jasnell Jun 3, 2016

Fishrock123 Jun 3, 2016

Fishrock123 commented Jun 3, 2016

Fishrock123 commented Jun 15, 2016

jasnell commented Jun 15, 2016

jasnell commented Jun 21, 2016

Fishrock123 Jun 27, 2016

Fishrock123 commented Jun 27, 2016

jasnell commented Jun 27, 2016

Fishrock123 commented Jun 28, 2016

jasnell commented Jun 28, 2016


		[the Node.js Event Loop]: ../topics/the-event-loop-timers-and-nexttick.html

doc: general improvements to timers.md #6937

doc: general improvements to timers.md #6937

Conversation

jasnell commented May 23, 2016

Checklist

Affected core subsystem(s)

Description of change

Choose a reason for hiding this comment

eljefedelrodeodeljefe commented May 23, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mscdex commented May 24, 2016

Fishrock123 May 24, 2016 • edited Loading

Choose a reason for hiding this comment

Fishrock123 commented May 24, 2016

jasnell commented May 24, 2016

bengl commented May 27, 2016

jasnell commented May 27, 2016

bengl commented May 27, 2016

jasnell commented May 28, 2016

jasnell commented May 28, 2016

bengl commented May 28, 2016

benjamingr commented May 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell commented Jun 2, 2016

jasnell commented Jun 3, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fishrock123 commented Jun 3, 2016

Fishrock123 commented Jun 15, 2016

jasnell commented Jun 15, 2016

jasnell commented Jun 21, 2016

Choose a reason for hiding this comment

Fishrock123 commented Jun 27, 2016

jasnell commented Jun 27, 2016

Fishrock123 commented Jun 28, 2016

jasnell commented Jun 28, 2016

Fishrock123 May 24, 2016 •

edited

Loading