consolidate EmitterIO args and validators to prevent duplicate arrays in options

[zepumph]

From #182 (comment)

We now are using Validator to validate Emitter args. The next step is to get rid of the duplicated arrays in argumentTypes:[*] and phetioType: EmitterIO([ * ])`.

To search for current places where this is a problem, search for the TODO:
// TODO: use of both of these is redundant, and should get fixed with https://github.com/phetsims/axon/issues/194

I think it should be solved in this issue instead, where it is specific to EmitterIO.

On top of just supporting the type value in objects passed to EmitterIO, I think we should streamline all of the keys. So we would move name and documentation into argumentTypes also (perhaps argumentTypes should be renamed?

For example, take this current implementation in PressListener:

    this._pressedEmitter = new Emitter( {
. . .
      // TODO: use of both of these is redundant, and should get fixed with https://github.com/phetsims/axon/issues/194
      argumentTypes: [
        { valueType: Event },
        { isValidValue: function( v ) { return v === null || v instanceof Node; } },
        { isValidValue: function( v ) { return v === null || typeof v === 'function'; } }
      ],
      phetioType: EmitterIO( [
         { name: 'event', type: EventIO },
         { name: 'targetNode', type: VoidIO },
         { name: 'callback', type: VoidIO }
       ] ),
. . .
   } );

could become something like:

this._pressedEmitter = new Emitter( {

  . . . 
  // TODO: use of both of these is redundant, and should get fixed with https://github.com/phetsims/axon/issues/194
  argumentTypes: [
    {
      name: 'event',
      phetioType: EventIO,
      valueType: Event
    },
    {
      name: 'targetNode',
      phetioType: VoidIO,
      isValidValue: function( v ) { return v === null || v instanceof Node; }
    },
    {
      name: 'callback',
      phetioType: VoidIO,
      isValidValue: function( v ) { return v === null || typeof v === 'function'; }
    }
  ]
    . . .   
} );

Then in Emitter, we could create the actual instance of EmitterIO that is needed.

In some cases, we may be able to get away with gathering the valueType from the phetioType, for example EventIO can have a pointer to its "core type" which is Event, so we don't need both of those. One downside of this is that we now can't factor out EmitterIO types. @samreid how might we get around that? Can you think of anything else that I'm missing from our conversation last week? I remember specifically talking about how we will have EITHER phetioType OR valueType, but the VoidIO example complicates that I think.

[samreid]

I wonder if it is preferable to bundle things in the EmitterIO array?

    this._pressedEmitter = new Emitter( {
. . .
      phetioType: EmitterIO( [
         { name: 'event', type: EventIO, valueType: Event },
         { name: 'targetNode', type: VoidIO, isValidValue: function( v ) { return v === null || v instanceof Node; } },
         { name: 'callback', type: VoidIO, isValidValue: function( v ) { return v === null || typeof v === 'function'; } }
       ] ),
. . .
   } );

@zepumph if that sounds good to you, let me know and I'll move forward implementing it.

[zepumph]

That feels less good to me.

I really dislike that in order create an Emitter you have to create an EmitterIO. We looked into that in #182 early on, and even implemented, but it just seems cleaner to me if move EmitterIO related stuff into argumentTypes.

We can talk more in person if you'd like.

[samreid]

I really dislike that in order create an Emitter you have to create an EmitterIO.

Uninstrumented code would use argumentTypes. When instrumenting to PhET-iO, it would be upconverted to phetioType: EmitterIO(...). We would have an assertion that argumentTypes and phetioType are mutually exclusive.

[samreid]

In discussion today, @zepumph agreed the proposal in #204 (comment) sounds reasonable and warrants investigation.

[samreid]

We also discussed that if the phetioType already has IO type, then valueType is redundant and can be dropped. Sounds reasonable! Warrants discussion and investigation.

[samreid]

For instance, in DOMEventIO, we would eliminate

isInstance: function( instance ) { return instance instanceof window.Event; },

and move toward a Validator-centric solution like:

validatorOptions: {valueType: window.Event},

[zepumph]

Marking high priority, since this is currently a gap that only get harder to fill as time goes on.

[zepumph]

Steps for this issue:

• isInstance methods on TypeIOs should be converted to validatorOptions
• Usages of argumentTypes for phet-io instrumented instances should be changed to use EmitterIO, using // TODO: use of both of these is redundant, and should get fixed to search for the 30 usages or so.

[samreid]

After today's discussion, will we use validator instead of validatorOptions?

[samreid]

I wrote on slack:

Sam Reid [7:08 AM]
I’m seeing numerous CT issues related to Assertion failed: value should be instanceof Event, value=[object Object]. I can reproduce by selecting an item in a ComboBox

It seems like one of the validators is expecting native Event but receiving scenery Event.
Looks related to #204, I’m committing a proposed fix that alleviates the symtoms in Molarity.

[samreid]

This validator doesn't seem correct:

  function ArithmeticModel( tandem, options ) {
    var self = this;

    // @private - for PhET-iO
    this.checkAnswerEmitter = new Emitter( {
      tandem: tandem.createTandem( 'checkAnswerEmitter' ),

      phetioType: EmitterIO( [ { name: 'results', type: ObjectIO, validator: { validValue: Object } } ] )
    } );

Because validValue is not a legal key for a validator. Same problem in Animation.js. It seems these were meant to use valueType.

[samreid]

These changes will also allow us to replace all of the assertInstanceOf calls with validator declarations on the types. I'm investigating that and will probably commit as part of this issue.

[zepumph]

• validateValidator should assert out if there are no expected keys (valueType, validValues, isValidValue).

[samreid]

I see your point about NullableIO.validator. Maybe we need to define something like isValid(instance,validator) which will help us here. Then NullableIO would be written:

        isValidValue: function( instance ) {
          return instance === null || isValid( instance, ioType.validator );
        }

Or maybe validate should return a boolean?

[samreid]

@zepumph and I reviewed the existing code and made numerous changes in the preceding commits. Tests are working, we have improved feature coverage without sacrificing good assertion messages. The strategy of passing through {assertions: true} is unconventional but we didn't see a preferable strategy.

We marked one TODO regarding garbage.

[samreid]

@zepumph and I finished reviewing and committing, and decided this issue is complete.