on('attached') registration in an on('attached') block fires immediately

[mattheworiordan]

Easier to show in code

channel.on('attached', () => {
  console.log('First attach');
  channel.on('attached', () => {
     console.log('Second attach');
  }
});

/* or */

channel.attach((err) => {
  console.log('First attach');
  channel.on('attached', () => {
     console.log('Second attach');
  }
});

/* or */

channel.attach((err) => {
  console.log('First attach');
  channel.once('attached', () => {
     console.log('Second attach');
  }
});

Both of these result in the following, even though there is only one attach:

=> First attach
=> Second attach

I don't think it's valid that an event handler, registered in the listener for an event should fire immediately. I think it's pretty clear that the developer wants that to happen the next time that event happens, not this time.

However, this does work as expected:

channel.once('attached', () => {
  console.log('First attach');
  channel.on('attached', () => {
     console.log('Second attach');
  }
});

=> First attach

[mattheworiordan]

Should the ordering of event registrations / emitting be part of the 0.10 spec perhaps?

[SimonWoolf]

This is because it's appending to an array while iterating through it. Fix would be for emit() to copy the listeners arrays before it starts. I'd be fine with a spec item to enforce that if you think it's necessary.

[mattheworiordan]

Yup, see https://github.com/ably/ably-ruby/blob/master/lib/ably/modules/event_emitter.rb#L82 and https://github.com/ably/ably-ruby/blob/master/spec/unit/modules/event_emitter_spec.rb#L73-L101, I clone it before iterating over and emitting.

I personally think this should be part of the spec, but open to feedback

[paddybyers]

The spec should say that every listener that is registered at the time of the call should called exactly once. It shouldn't say anything about cloning arrays of listeners. Other languages have other ways of meeting that requirement.

[SimonWoolf]

Fixed in 0.9 by a2fb1e2

(Not planning to fix in 0.8 as it is arguably a breaking api change; let me know if you disagree)

[paddybyers]

it is arguably a breaking api change

Well, I think only in the same way that any bugfix is potentially a breaking change for somebody.

[SimonWoolf]

Well, I think only in the same way that any bugfix is potentially a breaking change for somebody.

This isn't like a crash or a race condition; the previous behaviour was stable and predictable, so isn't it plausible that someone have built an app that relies on it (albeit probably unintentionally)?

[paddybyers]

I'm say that people can come to depend on all sorts of out-of-spec behaviours, but that shouldn't stop us fixing them, in general.

In this case I think the adverse impact of leaving it is small, and the likelihood we break someone's code by fixing it is smaller still.

[SimonWoolf]

OK, fair enough. Backported to master as 6f5f54a and 3b315e6

[mattheworiordan]

FYI, PR for this now at ably/docs#240