docs: Clearly document the behavior of ee.once(). #6371

lance · 2016-04-25T13:42:39Z

Checklist

documentation is changed or added
the commit message follows commit guidelines

Affected core subsystem(s)

doc

Description of change

Addresses #5566. The ee.once() function is currently documented as
invoking the listener, and then removing it when the event is
triggered. However, this is not really the case. The listener is removed
and then invoked. This only matters in a narrow set of use cases, but
when it matters, it matters that the docs are correct.

See the issue (#5566) for a discussion on why the code has not been
modified to match the documentation, but instead the documentation has
been modified to match the code.

cjihrig · 2016-04-25T13:48:21Z

doc/api/events.md

@@ -130,7 +130,7 @@ myEmitter.emit('event');
 ```

 Using the `eventEmitter.once()` method, it is possible to register a listener
-that is immediately unregistered after it is called.
+that is unregistered as it is called.


as it is called is still a bit ambiguous. Could you use before here?

cjihrig · 2016-04-25T13:48:30Z

LGTM with a comment.

eljefedelrodeodeljefe · 2016-04-25T14:42:15Z

LGTM

addaleax · 2016-04-25T14:53:21Z

LGTM but you may want to use doc: for your commit message; and if you want to reference the issue in it, I think it’s generally preferred to use the full github URL. You can also add an Fixes: header, I think.

lance · 2016-04-25T15:05:25Z

@addaleax yeah, I committed with 'docs:' instead of 'doc:'. I was aware that Fixes: was available, but since I can't really close the issue, I just referenced it. Do you need these things to change before merging the PR?

jasnell · 2016-04-25T15:14:58Z

@lance ... can you take a look at the new prependOnceListener() method description in the docs and see if it requires similar treatment?

addaleax · 2016-04-25T15:17:17Z

@lance No, all of that can be taken care of when landing the commit. 😄

Addresses #5566. The `ee.once()` function is currently documented as invoking the listener, and then removing it when the event is triggered. However, this is not really the case. The listener is removed and _then_ invoked. This only matters in a narrow set of use cases, but when it matters, it matters that the docs are correct. See the issue (#5566) for a discussion on why the code has not been modified to match the documentation, but instead the documentation has been modified to match the code.

@cjihrig

Based on comments from @cjihrig.

Additional documentation changes required for `ee.prependOnceListener` as per #5566.

lance · 2016-04-25T15:44:46Z

@jasnell took a look and updated the documentation. Based on my reading of lib/events.js it seems the behavior would be identical.

addaleax · 2016-04-25T15:49:01Z

doc/api/events.md

-*beginning* of the listeners array. This listener is invoked only the next time
-`eventName` is triggered, after which it is removed.
+*beginning* of the listeners array. This listener is removed, and then invoked
+only the next time `eventName` is triggered.


To me this sounds a bit like the listener is removed instantly after being added. Maybe just copy the corresponding sentence for .once() from above?

@addaleax

Address comments from @addaleax. Fixes: #5566

jasnell · 2016-04-25T16:45:06Z

LGTM

ChALkeR · 2016-04-25T18:45:17Z

LGTM

Addresses #5566. The `ee.once()` function is currently documented as invoking the listener, and then removing it when the event is triggered. However, this is not really the case. The listener is removed and _then_ invoked. This only matters in a narrow set of use cases, but when it matters, it matters that the docs are correct. See the issue (#5566) for a discussion on why the code has not been modified to match the documentation, but instead the documentation has been modified to match the code. Fixes: #5566 PR-URL: #6371 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Robert Lindstaedt <robert.lindstaedt@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>

jasnell · 2016-04-27T05:51:39Z

Landed in 23818c6.
Fixed up the commit message a bit.

Addresses #5566. The `ee.once()` function is currently documented as invoking the listener, and then removing it when the event is triggered. However, this is not really the case. The listener is removed and _then_ invoked. This only matters in a narrow set of use cases, but when it matters, it matters that the docs are correct. See the issue (#5566) for a discussion on why the code has not been modified to match the documentation, but instead the documentation has been modified to match the code. Fixes: #5566 PR-URL: #6371 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Robert Lindstaedt <robert.lindstaedt@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>

MylesBorins · 2016-05-18T00:08:29Z

does not land cleanly on v4.x-staging. Would someone be willing to backport?

MylesBorins · 2016-06-01T22:49:41Z

I'm going to mark this as don't land @lance / @nodejs/documentation please feel free to open a backport PR

lance · 2016-06-02T03:30:14Z

@thealphanerd - submitted backport PR here #7103

I have not submitted anything for backport before. I hope it's all up to snuff.

Addresses #5566. The `ee.once()` function is currently documented as invoking the listener, and then removing it when the event is triggered. However, this is not really the case. The listener is removed and _then_ invoked. This only matters in a narrow set of use cases, but when it matters, it matters that the docs are correct. See the issue (#5566) for a discussion on why the code has not been modified to match the documentation, but instead the documentation has been modified to match the code. Fixes: #5566 Ref: #6371 PR-URL: #7103 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Robert Lindstaedt <robert.lindstaedt@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>

cjihrig reviewed Apr 25, 2016
View reviewed changes

mscdex added doc Issues and PRs related to the documentations. events Issues and PRs related to the events subsystem / EventEmitter. labels Apr 25, 2016

lance added 3 commits April 25, 2016 11:43

doc: Clarity about timing for once listeners.

0fa1c90

Based on comments from @cjihrig.

doc: Clarify behavior of ee.prependOnceListener.

dc1049f

Additional documentation changes required for `ee.prependOnceListener` as per #5566.

addaleax reviewed Apr 25, 2016
View reviewed changes

doc: Clarify wording for ee.prependOnceListener.

4102e92

Address comments from @addaleax. Fixes: #5566

jasnell added this to the 6.0.0 milestone Apr 25, 2016

Fishrock123 removed this from the 6.0.0 milestone Apr 25, 2016

estliberitas force-pushed the master branch 2 times, most recently from 7da4fd4 to c7066fb Compare April 26, 2016 05:23

jasnell added the lts-watch-v4.x label Apr 27, 2016

jasnell closed this Apr 27, 2016

This was referenced May 4, 2016

Propose v6.1.0 wrong base please ignore #6556

Closed

Propose v6.1.0 (Openssl update) #6557

Merged

MylesBorins added the dont-land-on-v4.x label Jun 1, 2016

MylesBorins removed the lts-watch-v4.x label Jun 1, 2016

lance mentioned this pull request Jun 2, 2016

doc: Backport ee.once doc clarifications to 4.x. #7103

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

docs: Clearly document the behavior of ee.once(). #6371

docs: Clearly document the behavior of ee.once(). #6371

lance commented Apr 25, 2016

cjihrig Apr 25, 2016

cjihrig commented Apr 25, 2016

eljefedelrodeodeljefe commented Apr 25, 2016

addaleax commented Apr 25, 2016

lance commented Apr 25, 2016

jasnell commented Apr 25, 2016

addaleax commented Apr 25, 2016

lance commented Apr 25, 2016

addaleax Apr 25, 2016

jasnell commented Apr 25, 2016

ChALkeR commented Apr 25, 2016

jasnell commented Apr 27, 2016

MylesBorins commented May 18, 2016

MylesBorins commented Jun 1, 2016

lance commented Jun 2, 2016

docs: Clearly document the behavior of ee.once(). #6371

docs: Clearly document the behavior of ee.once(). #6371

Conversation

lance commented Apr 25, 2016

Checklist

Affected core subsystem(s)

Description of change

cjihrig Apr 25, 2016

Choose a reason for hiding this comment

cjihrig commented Apr 25, 2016

eljefedelrodeodeljefe commented Apr 25, 2016

addaleax commented Apr 25, 2016

lance commented Apr 25, 2016

jasnell commented Apr 25, 2016

addaleax commented Apr 25, 2016

lance commented Apr 25, 2016

addaleax Apr 25, 2016

Choose a reason for hiding this comment

jasnell commented Apr 25, 2016

ChALkeR commented Apr 25, 2016

jasnell commented Apr 27, 2016

MylesBorins commented May 18, 2016

MylesBorins commented Jun 1, 2016

lance commented Jun 2, 2016