docs,events: ee.once() event ordering differs from documentation

[omsmith]

• 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)

[zeusdeux]

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 that listener is removed *then* called since that might cause confusion.

[omsmith]

It's also worth noting this has been the behaviour since it was introduced (but I would also prefer fixing the ordering).

[MylesBorins]

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]

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.

On 04-Mar-2016, at 19:31, Myles Borins notifications@github.com wrote:

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?

—
Reply to this email directly or view it on GitHub.

[lance]

I was going to submit a PR with @zeusdeux suggested change, but this ends up breaking one of the removeListener tests. Specifically, here we get 'removeListener' instead of 'hello'. This is because the function executing from the original call to once() 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 a removeListener event before the function provided to once. This ensures user-land code can call once('removeListener', ...); from within the body of the original function supplied to once(), and still not receive the 'removeListener' event that is generated by once(). By moving the call to removeListener() to here, user land code ends up receiving the removeListener event that is generated.

I tried changing the removeListener function to not issue a removeListener event here and here if type === 'removeListener', but that ends up breaking expectations here where the removeListener event 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]

@lance @thealphanerd Just a doc update makes sense to me

[jasnell]

+1 to doc update.

[jasnell]

@nodejs/documentation