Repository navigation
docs,events: ee.once() event ordering differs from documentation #5566
Description
- Version: v4.3.2
- Platform: Linux osmith-ubuntu-t1700 4.2.0-30-generic docs: re-word project messaging #36-Ubuntu SMP Fri Feb 26 00:58:07 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
- Subsystem: events, docs
Documentation describes ee.once() behaviour as (emphasis mine)
Adds a one time listener function for the event. This listener is invoked only the next time event is triggered, after which it is removed.
The actual current behaviour is for the listener to removed, and then invoked.
'use strict';
const assert = require('assert');
const EventEmitter = require('events');
const order = [];
new EventEmitter()
.on('removeListener', () => order.push('remove'))
.once('foo', () => order.push('invoke'))
.emit('foo');
process.on('exit', () => assert.deepEqual(order, ['invoke', 'remove']));assert.js:89
throw new assert.AssertionError({
^
AssertionError: [ 'remove', 'invoke' ] deepEqual [ 'invoke', 'remove' ]
at process.<anonymous> (/home/omsmith/ee-once-remove-order-test/index.js:13:33)
at emitOne (events.js:77:13)
at process.emit (events.js:169:7)
Activity
[-]docs,events: [/-][+]docs,events: ee.once() event ordering differs from documentation[/+]zeusdeux commentedon Mar 4, 2016
Confirmed on
v5.7.0. Either we move this to after this or update the docs. Imho, the code change makes more sense as opposed to saying thatlistener is removed *then* calledsince that might cause confusion.omsmith commentedon Mar 4, 2016
It's also worth noting this has been the behaviour since it was introduced (but I would also prefer fixing the ordering).
MylesBorins commentedon Mar 4, 2016
It looks like the documentation was added in 179a7f6 and has always mis documented the function.
Modifying the behavior would be a semver major change. Modifying the documentation would be a semver patch change which could be backported to v4 / v5. In reality the doc change should potentially happen even if the behavior is changed on master.
Is there an advantage to the behavior change aside from accurately following what was documented?
zeusdeux commentedon Mar 4, 2016
For one, it enforces logically correct ordering. You might want to, for example, log stuff after a handler is run for the last time just before being removed.
lance commentedon Mar 8, 2016
I was going to submit a PR with @zeusdeux suggested change, but this ends up breaking one of the
removeListenertests. Specifically, here we get 'removeListener' instead of 'hello'. This is because the function executing from the original call toonce()right here has not completed and so hasn't been removed yet.I can fix this test, but I wonder if this doesn't illustrate some potential brittleness to this change.
Update:
With the code as it is now,
this.removeListener()triggers aremoveListenerevent before the function provided toonce. This ensures user-land code can callonce('removeListener', ...);from within the body of the original function supplied toonce(), and still not receive the'removeListener'event that is generated byonce(). By moving the call toremoveListener()to here, user land code ends up receiving theremoveListenerevent that is generated.I tried changing the
removeListenerfunction to not issue aremoveListenerevent here and here iftype === 'removeListener', but that ends up breaking expectations here where theremoveListenerevent is expected.It seems to me, that unless you are willing to accept a possibly breaking change, the best course is to just change the documentation. Two node.js tests expect behavior as it is today. You can be sure user code does as well.
omsmith commentedon Apr 11, 2016
@lance @thealphanerd Just a doc update makes sense to me
jasnell commentedon Apr 19, 2016
+1 to doc update.
jasnell commentedon Apr 22, 2016
@nodejs/documentation
doc: correctly document the behavior of ee.once().
doc: Backport ee.once doc clarifications to 4.x.
doc: Backport ee.once doc clarifications to 4.x.
doc: correctly document the behavior of ee.once().