Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Events / Observables #71

Closed
liberty-rowland opened this issue Mar 13, 2016 · 48 comments
Closed

Events / Observables #71

liberty-rowland opened this issue Mar 13, 2016 · 48 comments

Comments

@liberty-rowland
Copy link
Contributor

I'd be interested in discussing how expected events on observable objects could be described. Here's a first draft for one possibility based on my understanding of the current structural types:

new Channel({ displayName: String }) => ChannelInstance

interface ChannelInstance {
  displayName: String
}, emits: {
  messageAdded: { body: String, authorId: Number },
  memberJoined: { id: Number }
}

Thoughts?

@ericelliott
Copy link
Owner

How does a user subscribe to those events?

@liberty-rowland
Copy link
Contributor Author

In common usecases I've seen: either MDN's addEventListener or EventEmitter's: [on, once, addListener].

In the former case, I can see specifying an addEventListener method on the interface but for the latter there might be considerable duplication.

@liberty-rowland
Copy link
Contributor Author

In JSDoc the events emitted are enumerated with @fires and the events are described by @event:

/**
 * Represents a Channel on the server.
 * @fires Channel#memberJoined
 */
var Channel = function() {
    // ...
};

/**
 * Member joined the Channel.
 * @event Channel#memberJoined
 * @type {object}
 * @property {Number} id - The joining member's ID.
 */

Perhaps another configuration could be:

interface Channel { }, fires: memberJoined, messageAdded

event memberJoined {
  id: Number
}

event messageAdded {
  authorId: Number,
  body: String
}

My only concern here is multiple objects could share events with the same name. For instance, Channel#left and Member#left.

@ericelliott
Copy link
Owner

I like emits. It would be a mistake to include this without specifying the event API, though (.on, etc...). =)

@liberty-rowland
Copy link
Contributor Author

So, 3 implementations come to mind:

1. Implement extends:

new Channel({ displayName: String }) => ChannelInstance

interface ChannelInstance extends EventEmitter {
  displayName: String
}, emits: {
  messageAdded: { body: String, authorId: Number },
  memberJoined: { id: Number }
}

interface EventEmitter {
  on: ({ eventName: String, handler: Function }) => Void,
  once: ({ eventName: String, handler: Function }) => Void,
  addListener: ({ eventName: String, handler: Function }) => Void
}

The drawback there is that it's not guaranteed or implied that any of, or which of, the extended classes is the event emitter.

2. emits from

new Channel({ displayName: String }) => ChannelInstance

interface ChannelInstance {
  displayName: String
}, emits from EventEmitter: {
  messageAdded: { body: String, authorId: Number },
  memberJoined: { id: Number }
}

interface EventEmitter {
  on: ({ eventName: String, handler: Function }) => Void,
  once: ({ eventName: String, handler: Function }) => Void,
  addListener: ({ eventName: String, handler: Function }) => Void
}

This is basically a special invocation of extends that specifically calls out the event emitter.

3. Be opinionated

We could take the opinion that any interface should only contain a single interface for events and create a special observable keyword that defines what an observable class looks like (Automatically applied to interfaces that emit):

observable {
  on: ({ eventName: String, handler: Function }) => Void,
  once: ({ eventName: String, handler: Function }) => Void,
  addListener: ({ eventName: String, handler: Function }) => Void
}

new Channel({ displayName: String }) => ChannelInstance

interface ChannelInstance {
  displayName: String
}, emits: {
  messageAdded: { body: String, authorId: Number },
  memberJoined: { id: Number }
}

@ericelliott
Copy link
Owner

interface ChannelInstance extends EventEmitter {

No. That's not always what's happening. See the "composing types" PR.

@liberty-rowland
Copy link
Contributor Author

Ah, excellent. So the spread operator might be useful here, too. The first implementation would look more like:

interface ChannelInstance {
  ...EventEmitter,
  displayName: String
}, emits: {
  messageAdded: { body: String, authorId: Number },
  memberJoined: { id: Number }
}

Alternatively, the spread operator could be overloaded in the case of the emits clause for more clarity:

interface ChannelInstance {
  displayName: String
}, emits: {
  ...EventEmitter,
  messageAdded: { body: String, authorId: Number },
  memberJoined: { id: Number }
}

@ericelliott
Copy link
Owner

This form looks correct to me...

interface ChannelInstance {
  ...EventEmitter,
  displayName: String
}, emits: {
  messageAdded: { body: String, authorId: Number },
  memberJoined: { id: Number }
}

@Mouvedia
Copy link
Collaborator

Since we are doing documentation, we will need to have some way to add the model boolean. Of course that only matters for a DOM-like environment, which is pretty common.

  • false bubbling (default)
  • true capturing

@ericelliott
Copy link
Owner

I'm not sure we need to do that. Seems like something that should be called out in prose and explained in more depth, anyway...?

@Mouvedia
Copy link
Collaborator

That was just an idea I was throwing. Maybe it's going too far.

@tomek-he-him
Copy link
Collaborator

Can’t we achieve the same in the current spec without adding features (read: complexity)?

interface ChannelInstance {
  ...EventEmitter,
  displayName: String,
  on: ('message added', { body: String, authorId: Number }) => Void,
  on: ('member joined', { id: Number }) => Void,
}

By the way, an event emitter will differ in different contexts. A custom element will emit DOM events, whereas a node server will likely use EventEmitter – and both ways are idiomatic. It’d be wrong to tie ourselves to one context.

@ericelliott
Copy link
Owner

  on: ('message added', { body: String, authorId: Number }) => Void,
  on: ('member joined', { id: Number }) => Void,

This is not currently supported, and I find the syntax a bit ambiguous. How do you tell it apart from a normal function signature?

@tomek-he-him
Copy link
Collaborator

I meant it to be a normal overloaded function signature. I’m not an expert with our syntax yet though – chances are it should have been something like:

  on: {
    ('message added', { body: String, authorId: Number }) => Void,
    ('member joined', { id: Number }) => Void,
  },

@ericelliott
Copy link
Owner

I meant it to be a normal overloaded function signature.

That makes a lot more sense, and yes, that would work fine, except that different implementations might have different subscribe methods, so if we rely strictly on this, it doesn't necessarily scream "this is an event emitter".

@tomek-he-him
Copy link
Collaborator

The point I’m trying to make is:

Do we really need to scream “this is an event emitter”, since there are so many valid tastes of event emitters and since our current syntax covers all of them already?

@ericelliott
Copy link
Owner

I suppose that's a good question.

@liberty-rowland
Copy link
Contributor Author

Looking to clarify re:

on: {
  ('message added', { body: String, authorId: Number }) => Void,
  ('member joined', { id: Number }) => Void,
},

Is on meant to be a special method name reserved for describing events? Or would this need to be duplicated for [on, once, addListener, removeListener, off] (or whatever array of methods the implementation uses)?

@ericelliott
Copy link
Owner

Is on meant to be a special function name reserved for describing events? Or would this need to be redefined for [on, once, addListener, removeListener, off]?

The latter, though I don't see why it would be necessary to redefine each event for each corresponding method...

@liberty-rowland
Copy link
Contributor Author

I understand the desire to re-use existing functionality, and I find overloading by event name and handler signature creative and efficient.

But I have concerns:

  1. I have to repeat myself.
  2. It feels strange exposing class-level events as method overrides.
  3. It will be hard for doc generators to be explicit about defining events.

I guess it comes down to whether we consider the observable pattern to be important enough to facilitate explicit documentation. I do.

The latter, though I don't see why it would be necessary to redefine each event for each corresponding method...

I suppose it wouldn't, but then (unless I'm missing something) we're defining one method with all of these overloads, and hoping it's somehow implied that any other given method is or is not related to listening to those events.

@ericelliott
Copy link
Owner

I have to repeat myself.

How do you have to repeat yourself? Can you show me an example?

It feels strange exposing class-level events as method overrides.

They're not method overrides. It's the function polymorphism syntax, slightly abused to create separate signatures for each of the event-type values. ;)

It will be hard for doc generators to be explicit about defining events.

This is one of my concerns, which is why I thought an explicit emits might be preferable, but if we can figure out how to tell the doc generator that a block is a bunch of events, that could solve the problem.

...hoping it's somehow implied that any other given method is or is not related to listening to those events.

This happens anyway with the emits syntax... only worse. In that case, the user has no way of knowing how the events are subscribed to! =)

@liberty-rowland
Copy link
Contributor Author

How do you have to repeat yourself? Can you show me an example?

Well, I guess I wouldn't have to but I don't think the alternative is much better:

on: {
  ('message added', { body: String, authorId: Number }) => Void,
  ('member joined', { id: Number }) => Void,
},
once: {
  ('message added', { body: String, authorId: Number }) => Void,
  ('member joined', { id: Number }) => Void,
}

or

on: {
  ('message added', { body: String, authorId: Number }) => Void,
  ('member joined', { id: Number }) => Void,
},
once: (String, ()) => Void

(Not quite sure how once would look in the second example)

This is one of my concerns, which is why I thought an explicit emits might be preferable, but if we can figure out how to tell the doc generator that a block is a bunch of events, that could solve the problem.

This happens anyway with the emits syntax... only worse. In that case, the user has no way of knowing how the events are subscribed to! =)

I see that both approaches have a similar problem. With emits it's not explicit about which methods are associated with the events, although it is explicit what the events are. With the polymorphism approach, one method is specifically called out as an event handler, but the events themselves are not explicit and any other event handler methods would need to contain duplicated information if they are to be explicit as well (They can exist without, but they won't be explicitly related).

I think either approach can be modified to be appropriate:

  • emits would need to be accompanied by some mechanism to denote which methods are tied to the events
  • creative polymorphism approach would need to be accompanied by a way to denote which method describes to events, and which other methods use the same set of events (by pointing to the first method somehow, probably)

For emits, my suggestion is to include the event interface in the emits clause rather than the interface, something like:

interface { }, emits: { ...EventEmitter }
// or
interface { }, observable: EventEmitter { }

For the alternative, perhaps something like this would be enough:

interface {
  [on, once, addListener]: {
    ('messageAdded', { body: String, authorId: Number }) => Void,
    ('memberJoined', { id: Number }) => Void,
  },
  [off, removeListener]: ({ name: String }) => Void
}

Although the latter would probably result in duplicate documentation of the same events for each method name, and I guess they're still not explicitly event related.

@ericelliott
Copy link
Owner

If you use the object spread syntax to mix in the emitter's natural API, you don't even have to do that.

interface Thing {
  ...EventEmitter,
  on: {
    ('message added', { body: String, authorId: Number }) => Void,
    ('member joined', { id: Number }) => Void,
  }
}

@liberty-rowland
Copy link
Contributor Author

As a sentient being I can divine how that works and that looks pretty elegant to me, but I think there's a missing link as far as a documentation generator.

You're right. We would just need to figure out how to denote that on is describing a list of events. Maybe just a symbol prefix/suffix?

@Mouvedia
Copy link
Collaborator

We need a function signature example.

(paramName: Type)
   => Type,
   throws: Error,
   emits: dragenter, dragleave

Is this correct?

@liberty-rowland
Copy link
Contributor Author

@Mouvedia Although the function may trigger an event, the event is actually emitted from the class.

@Mouvedia
Copy link
Collaborator

@ryan-rowland sorry could you be more precise? What are you calling a "class"?

@liberty-rowland
Copy link
Contributor Author

@Mouvedia Can you give more context to your example code snippet?

@Mouvedia
Copy link
Collaborator

Mouvedia commented Apr 15, 2016

OK I see what you mean now: the event can't be simulated so it's not actionnable.

Is it made for things like the Push API?

@ericelliott
Copy link
Owner

I think it's important we remain consistent with the throws notation for events, and emits is probably the best way to do that.

IMO, we don't need to explicitly spell out the event API -- just mix in the event emitter with the object spread syntax.

I propose:

interface MyEventBus {
  ...EventEmitter
}, emits: {
  ('message added', { body: String, authorId: Number }),
  ('member joined', { id: Number })
}

Note: I think it's obvious that Void is implied in these cases, so it's a good call to omit it.

@Mouvedia
Copy link
Collaborator

Shouldn't this part

emits: {
  ('message added', { body: String, authorId: Number }),
  ('member joined', { id: Number })
}

be

emits: {
  'message added': { body: String, authorId: Number },
  'member joined': { id: Number }
}

instead?

@ericelliott
Copy link
Owner

ericelliott commented Apr 18, 2016

Maybe. What does everybody think of that?

I think it could be confusing, because some event emitters support any number of arguments. I've always thought of it as a function call (because it translates into a callback function call)... so I see it as a function signature for the callback, minus the implied => Void:

(eventType: String, arg1: Type, arg2: Type, ...rest: Type)

@Mouvedia
Copy link
Collaborator

Do you have a non-node.js example?

@ericelliott
Copy link
Owner

ericelliott commented Apr 18, 2016

Example from Backbone docs:

interface Alerter: {}, emits: {
  ('alert', msg: String)
}
var object = {};

_.extend(object, Backbone.Events);

object.on("alert", function(msg) {
  alert("Triggered " + msg);
});

object.trigger("alert", "an event");

jQuery events, custom DOM events, any universal EventEmitter usage are all commonly used in browsers.

@Mouvedia
Copy link
Collaborator

Mouvedia commented Apr 18, 2016

(eventType: String, arg1: Type, arg2: Type ...rest: Type)

Is there a , missing?

@ericelliott
Copy link
Owner

ericelliott commented Apr 18, 2016

yes. This is why we need a working parser.

@liberty-rowland
Copy link
Contributor Author

My 2 cents:

emits: {
  ('message added', { body: String, authorId: Number }),
  ('member joined', { id: Number })
}

This seems like an odd hybrid to me. The definitions seem to be describing overloaded arguments for the on method, but I thought that made more sense when it was proposed as actually describing an overloaded on method. In its current state it looks like we're wrapping an array of methods inside object literal notation, which throws me.

emits: {
  'message added': { body: String, authorId: Number },
  'member joined': { id: Number }
}

I prefer the above because the intention is a littler clearer. We're saying "Here's a map of what this class emits, to the arguments passed to its listeners" in a way that's truer to JSON standards.

@ericelliott
Copy link
Owner

ericelliott commented Apr 18, 2016

I prefer the above because the intention is a littler clearer. We're saying "Here's a map of what this class emits, to the arguments passed to its listeners" in a way that's truer to JSON standards.

Remember, there can be any number of arguments emitted with an event. We literally are describing a function signature when we describe events, because they are always triggered via callback (at least, they are in every popular implementation currently in use).

This is what the spec needs to capture:

(eventName: String, arg1: Type, arg2: Type, ...rest: Type)

In other words, this doesn't work because it's ambiguous:

emits: {
  'message added': { body: String, authorId: Number },
  'member joined': { id: Number }
}

Could mean:

emits: {
  // one event
  ('message added': { body: String, authorId: Number }, 'member joined': { id: Number })
}

I don't think that's what you meant by your example, but how can a parser tell the difference?

@liberty-rowland
Copy link
Contributor Author

liberty-rowland commented Apr 18, 2016

Ah. I see where we're missing each other. I've been using the curly brackets to denote multiple parameters, not a single object.

emits: {
  'messageAdded': { body: String, authorId: Number },
  'memberJoined': { id: Number }
}

Was meant to represent

class.on('messageAdded', function(body, authorId) { /* ... */ });
class.on('memberJoined', function(id) { /* ... */ });

So perhaps clearer notation would be:

emits: {
  'messageAdded': (body: String, authorId: Number),
  'memberJoined': (id: Number)
}

@ericelliott
Copy link
Owner

ericelliott commented Apr 18, 2016

This is acceptable to me if everybody else likes it better:

emits: {
  'messageAdded': (body: String, authorId: Number),
  'memberJoined': (id: Number, { Name: String, email: String })
}

I guess that makes sense if you think of it this way:

messageAdded is the name of the callback function with the signature (body: String, authorId: Number).

This fits our existing method syntax:

emits: {
  messageAdded(body: String, authorId: Number),
  memberJoined(id: Number, { Name: String, email: String })
}

I'm not sure if we've documented it, but IMO, it should be perfectly legal to use quotes and colons with the method syntax, too:

interface MyCallbacks {
  'messageAdded': (body: String, authorId: Number),
  'memberJoined': (id: Number, { Name: String, email: String })
}

@liberty-rowland
Copy link
Contributor Author

liberty-rowland commented Apr 18, 2016

@ericelliott Pretty sure I'm +1 to:

emits: {
  'messageAdded': (body: String, authorId: Number),
  'memberJoined': (id: Number, { Name: String, email: String })
}

To clarify, I'm interpreting this memberJoined as:

class.on('memberJoined', function(id, details) {
  details.hasOwnProperty('Name');
  details.hasOwnProperty('email');
});

Is that right?

@ericelliott
Copy link
Owner

ericelliott commented Apr 18, 2016

@ryan-rowland Looks like you got it.

@Mouvedia I don't understand what the {} at the end of those signatures is there for. They're just function signatures with the Void return type implied (imagine => Void at the end, if it helps).

@Mouvedia
Copy link
Collaborator

@ericelliott function expression vs function declaration

emits: {
  'messageAdded': (body: String, authorId: Number),
  'memberJoined': (id: Number, { Name: String, email: String })
}

// versus

emits: {
  messageAdded(body: String, authorId: Number),
  memberJoined(id: Number, { Name: String, email: String })
}

@ericelliott
Copy link
Owner

@Mouvedia I believe those forms should be considered interchangeable. AFAIK, there is no difference in the type produced by function expressions vs declarations in JS object definitions.

@tomek-he-him
Copy link
Collaborator

Late to the party, but I don’t really like where this went in the end. If I have an API like

interface MyEmitter {}, emits: {
  'messageAdded': (body: String, authorId: Number),
  'memberJoined': (id: Number, { Name: String, email: String })
}

how do I consume it?

myEmitter.on('messageAdded', () => {});
myEmitter.addEventListener('messageAdded', () => {});
myEmitter.messageAdded.subscribe(() => {});

All of these are valid ways to emit events in different environments: node / DOM / RxJS. And I’m sure there are more ways.

@tomek-he-him
Copy link
Collaborator

Besides, if we keep adding new features to the spec instead of looking for solutions which cover more cases (even if it means a character or two more to type, as in the overloading example), we’ll end up with scope creep.

The fatter the spec gets, the harder it’ll be for others to learn – and the less likely it is to adopt widely.

In the end, what spoke against using overloading for describing event emitters?

@ericelliott
Copy link
Owner

@tomekwi One of the most important goals for me was to create a standard for documenting APIs that we can use in technical documentation.

We needed a standard, easily machine-readable way to document events, regardless of the listening API.

As for scope creep, I don't expect early implementations to cover all the features, but event documentation was a must-have feature for me that needed to be in the MVP version. I'm already using it.

@Mouvedia
Copy link
Collaborator

Should we add an example featuring a constructor as well?
CustomEvent() comes to mind if we do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants