Should each Emitter instance be allowed to emit with any number of args?

[zepumph]

Right now there is complete flexibility on how many arguments you emit with for an emitter instance. So far there haven't seen to be any complaints about this loose system, but @samreid, @chrisklus, and @zepumph ran into difficulties with this for PhET-iO today.

Would this be safer and more maintainable if an Emitter instance was defined with a set number of args that it expected to emit with? Perhaps this would be too rigid of a system? Are there many places where a single emitter will use more than one emitting function in different use cases?

For example, here is something you can currently do with Emitter:

var e = new Emitter();

e.emit();

e.emit1( new Property());

e.emit3( 'hi', 'you', 'rock')

e.emit1( new Vector2( 3, 4) );

For obvious reasons, this seems brittle and difficult when it comes to adding listeners to this Emitter.

The reason this came up, was because we were trying to decide how EmitterIO should properly serialize Emitter's arg types. For example we have an EmitterIO.<BooleanIO> that is still technically allowed to emit3(), although that breaks across the phet-io api.

A "strong typing" solution (that we aren't necessarily pushing for) would be to have a different type for each number of args you plan to emit for, like Emitter extends BaseEmitter, Emitter1 extends BaseEmitter, Emitter2 extends BaseEmitter. Each type would only have the emitting method that corresponds to the number of args it is made for.

[zepumph]

Adding to developer meeting, although likely it would be nice to have a week of async discussion and thought gathering before bringing this to the team.

[samreid]

I appreciate the simplicity of a single Emitter type and I'm not aware of any misusages of it so far. Another option alluded to in #182 (comment) would be to pass an option to Emitter indicating the number and/or type of emit arguments. We kind of have something like that for PhET-iO in Emitter.options.phetioType.

[samreid]

Another possibility would be to convert everything to be emit1 and named as emit. When called with no arguments, then the argument is undefined. When called with 1 argument, it works as expected. To call with 2+ arguments, you would need to use an array or object.

[zepumph]

Another possibility would be to convert everything to be emit1 and named as emit. When called with no arguments, then the argument is undefined. When called with 1 argument, it works as expected. To call with 2+ arguments, you would need to use an array or object.

For PhET-iO would we then need a Nullable( phetioArgumentType || ArrayIO( phetioArgumentTypes ) ) typeIO to handle this flexibility across the frame?

[zepumph]

Also note that there are only 35 or so usages of emit2 and emit3. Much less than I would have thought.

[samreid]

For PhET-iO would we then need a Nullable( phetioArgumentType || ArrayIO( phetioArgumentTypes ) ) typeIO to handle this flexibility across the frame?

Well, each EmitterIO would be created with the appropriate type on a case-by-case basis (instead of thinking of one type that works for everything). For instance:

new Emitter({phetioType: EmitterIO(StringIO)});
new Emitter({phetioType: EmitterIO()}); // for no-args?
new Emitter({phetioType: EmitterIO(ArrayIO(Vector2IO))});

[pixelzoom]

Do Emitter instances typically use one of the emit* methods, or are there some cases where more than one is used?

[zepumph]

Do Emitter instances typically use one of the emit* methods, or are there some cases where more than one is used?

Seems like a hard question to answer definitively. I can say with relatively strong confidence that I have not emitted using multiple methods for the same emitter instance.

[pixelzoom]

Since all listeners are called when any emit method is called, I don't see how using more than 1 of the emit methods would not be buggy. And if it did work, it seems like a bad practice to use an Emitter for more than one purpose.

[zepumph]

Springboarding from that, perhaps the easiest solution to validate is to have an option of type {number} called something like numberOfArgs. Them emit3 can assert that numberOfArgs === 3.\

EDIT: this option may have a default of 0, but could be easily overridable.

[pixelzoom]

I'm now recalling the 2 things that I don't like about the current Emitter implementation.
(1) If you need to call a listener with N args, and emitN doesn't exist, you have to add it to Emitter.
(2) Nothing prevents you from using more than one of the emit methods.

What @zepumph proposed in #182 (comment) sounds like a reasonable way to prevent you from using more than one emit method.

[jonathanolson]

If the concern is serialization, couldn't emitters be given an array of phet-io types for the arguments?

[zepumph]

If the concern is serialization, couldn't emitters be given an array of phet-io types for the arguments?

Yes that is what we currently do (see Emitter.options.phetioType) and it works very nicely. I was brainstorming for a general solution such that uninstrumented sims wouldn't need to provide TypeIO arrays to each Emitter. Perhaps we could have the phetioType (if provided), override the numberOfArgs option, so that we don't need to worry about both ever (since they convey the same information).

[jbphet]

For the record, I've never had a case where I used different numbers of params from the same emitter.

[zepumph]

Decided in the developer meeting, we will update Emitter to only have one method, emit. When instantiating this Emitter, we will pass in a valueTypes array that holds the types expected in the emit call.

Then in the emit method we will assert that all arguments are of the correct length and types. This is hidden behind assert, so performance isn't a concern.

This code will mimic that of Property.js and the way it asserts the correct valueType.

For PhET-iO we will have a parallel array option phetioValueTypes that already exists.

I'll give this one a go.

[zepumph]

For the record, I've never had a case where I used different numbers of params from the same emitter.

(phet-io thanks you)

[zepumph]

I think this is safest to try to accomplish in steps, rather than just one super commit. I think I will do the following steps:

1. update the Emitter constructor to support the way of the future, but don't assert out if it isn't doing it (to support backwards compatability).
2. update the emit() function to support emit1, and transfer emit1 usages.
3. Do the same for emit2
4. Do the same for emit3.
5. Update Emitter so that it fails if you don't specify the appropriate valueTypes

[zepumph]

• I'm unsure about how to set the validValues for Property's changedEmitter.

[samreid]

Can they come from the Property's validValues? Or maybe we don't need to supply them because the Property.changeEmitter is not instrumented?

[zepumph]

Converting usages of emitN to emit + valueTypes option will change drastically based on our decision in #189, marking on hold until we figure that out.

[zepumph]

In #189 we have come up with a nice strategy for Property, and now need to apply this to Emitter. Afterwards we will will add phetioType to Validator.js in #194.

Marking off hold to try to convert usages currently using TypeDef and Emitter.argumentTypes over to use Validator.

[zepumph]

Thoughts while working on this:

• Should we rename "argumentTypes" to something like "listOfValidatorOptionsForArguments"
• Emitter didn't seem to have a need for pickOptions, but I'm wondering if Validator.validateOptions should error out if there are other, unexpected keys in the object passed in. To me that feel very strict, and we may run into problems with it, but it also could be a nice way to assert that things are happening as we expect. As discussed in last dev meeting, it is less idiomatic to pick options out of an object, and more appropriate to pass the whole thing to a parent and have the parent only use the options it is expecting, and to ignore the rest. The Property/Validator relationship is a bit different than the parent/child type one though.

Summary of work:
In the above commits I updated Emitter to use Validator. This involved

• removing any argumentTypes validation from EmitterIO
• Changing code in Emitter.js from TypeDef to Validator
• Deleting TypeDef
• Converting the 41 usages of emitters calling emit with an arg from depending on EmitterIO, to instead using argumentTypes with Validator-like options.

Overall I'm happy about this change, and I think, after a review, phet brand should be cleared to use this approach for creating emitters.

The main drag about the above commit is that it doesn't yet support phetioType as part of argumentTypes, which I think is the way to go so we don't have to support parallel arrays. But since we don't support that yet, for phet-io instrumented Emitters we have an array for argumentTypes, and one passed into the EmitterIO for the phetioType. Each of these has the same todo on it:
// TODO: use of both of these is redundant, and should get fixed with https://github.com/phetsims/axon/issues/194

I pointed to that issue instead of this one because that is above consolidating phetioType and the traditional "valueType" into a single place.

@samreid and I still need to support moving EmitterIO types into argumentTypes for Emitter specifically. I'll make a new issue for that.

[zepumph]

@samreid please review this, and note #204 as further work in combining phetioType into argumentTypes.

[zepumph]

I don't want un-reviewed code in master for too long. marking blocks publication.

[samreid]

@chrisklus @zepumph and I reviewed the preceding commits and comments. We have a few recommendations to commit, and will open two follow-up issues.

[samreid]

Review is complete, this is working nicely and follow-up work is lower priority and noted in other issues.