investigate factoring out PhET-iO data stream responsibilities from Emitter

[pixelzoom]

In phetsims/scenery#928, we discussed leveraging Emitter to add messages to the PhET-iO data stream. In phetsims/scenery#928 (comment) from 2/21/19 dev meeting, the consensus was that we'd prefer not to have this responsibility in Emitter, and it is worth taking time to investigate how that responsibility might be factored out.

@ariel-phet's instructions were:

• @pixelzoom will create this issue.
• @pixelzoom will create an initial proposal.
• @samreid, @zepumph, and @pixelzoom will discuss, starting with a Zoom call.
• Label for dev meeting when we have something concrete to discuss with the dev team.

[pixelzoom]

I made a straw man proposal in phetsims/scenery#928 (comment), and I'll probably start there.

[samreid]

One feature that I'm not sure how it will be covered by the given proposal: what if we have an event that we require for recording and playback (and hence it has a first or action sort of thing), and it also has listeners?

[pixelzoom]

Do you mean an Event or some other "event"? Example?

[samreid]

For example, any of the 24 phetioPlayback: true Emitters.

[pixelzoom]

@samreid said:

One feature that I'm not sure how it will be covered by the given proposal: what if we have an event that we require for recording and playback (and hence it has a first or action sort of thing), and it also has listeners?

I haven't made the proposal yet. But assuming we have a method named doPhetioTask that wraps a function in phetioStartEvent and phetioEndEvent, and Emitter also uses doPhetioTask when emit is called...

// If you want notification nested inside of task in the data stream
this.taskDoneEmitter = new Emitter(...);
doPhetioTask(..., (...) => {
   // do the task
   this.taskDoneEmitter.emit(...);
} );

// if you want notification to follow task in the data stream
this.taskDoneEmitter = new Emitter(...);
doPhetioTask(..., (...) => {
   // do the task
} );
this.taskDoneEmitter.emit(...);

This is more flexible than the current Emitter approach, because you can choose whether to combine or separate the task and the notification in the data stream.

[pixelzoom]

• @pixelzoom will create an initial proposal.

Sorry it's taken me awhile to get back to this, I've had other higher priority work. That said...

In phetsims/scenery#928 (comment), I showed a brute force (untested) way to separate Emitter into 2 classes that I called Emitter and (for lack of a better name) Task. Emitter is responsible only for notification of listeners, which was its original responsibility. Task is responsible for making some unit of work (a “task”) appear in the PhET-iO data stream.c Emitter would use Task when it does notification via its emit method. DragListener would use Task (instead of an Emitter with one listener, see this._draggedEmitter) in its drag method.

It’s not clear to me that Task needs to be a class. It could perhaps be a utility method. It depends on whether there's a need to be stateful (have this instance fields).

If Task is a class, it’s also not clear to me that Emitter should extend class. Composition might be preferable to inheritance. And composition would certainly be appropriate elsewhere, e.g. in DragListener and Sim.

So the proposal is:

• Factor out the responsibilities that are related to PhET-iO data stream and state, into either a class or utility method(s), as seems appropriate.
• Use the new class or method(s) in Emitter, DragListener, and other places where we’re currently using Emitter with a first option.
• For cases that involve public instances of Emitter, decide whether to nest notification inside the associated task, or to perform notification after completing the task. This determines nesting of messages in the data stream.

[samreid]

Use the new class or method(s) in Emitter, DragListener, and other places where we’re currently using Emitter with a first option.

Currently the PhET-iO playback engine only knows how to record and play back Emitter events. If we pursue the proposed strategy, we will need to make it so the PhET-iO playback engine can record and play back events from the new type.

[zepumph]

Factor out the responsibilities that are related to PhET-iO data stream and state, into either a class or utility method(s), as seems appropriate.

I think the idea of wrapping logic in phetioEvents through methods instead of a Type is interesting and creative. I don't want to say an immediate "no" to it. My hesitation lies in how Type-focused our instrumentation process has been thus far; it isn't clear to me immediately how we will be able to support wrapper side features (like emitting the data stream) without an instrumented type associated with that logic. The clearest example to me is to support phet-io playback. Since it is all tied to instances of Emitter, it is easy for the wrapper to invoke those calls from the wrapper side. If instead that even logic was tied to a function, I'm not sure how the wrapper side would be able to get a hook to that sim code to trigger that event again. Let's talk further!

---

From reading @pixelzoom's thoughts above, it is clear that we have adopted Emitter as the basis of the phet-io data stream in part because of convenience and its already large presence throughout the code base. There is no reason why we wouldn't be able to convert the draggedEmitter in DragListener to be a draggedPhetioTasker if desired, therefore making PhetioTasker (with a better name) the building block of phet-io data stream instead of Emitter. I think this has been stated before, but I'll say it again here for clarity. The reason this wasn't done earlier was because we were able to save a lot of development time by utilizing the tech that was already here (Emitter), and by adding complexity in that type to accomplish phet-io needs.

[pixelzoom]

Next on the checklist is:

@samreid, @zepumph, and @pixelzoom will discuss, starting with a Zoom call.

Let me know when you want to do this.

[pixelzoom]

We're discussing on Thu 4/4/19 @ 9:30am.

[samreid]

It’s not clear to me that Task needs to be a class. It could perhaps be a utility method.

If it is going to be PhET-iO instrumented for record and playback, then it will need to be an instance.

As a first pass, I'd recommend investigating renaming Emitter to Task and removing the listener functionality. Then creating a new subclass of Task called Emitter which adds listeners.

[pixelzoom]

That's basically what I did (untested) in phetsims/scenery#928 (comment).

[samreid]

Incremental progress from today's discussion:

Index: js/Action.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/Action.js	(date 1554395079000)
+++ js/Action.js	(date 1554395079000)
@@ -0,0 +1,165 @@
+// Copyright 2019, University of Colorado Boulder
+
+/**
+ * An action that can be sent to the data stream, and optionally recorded for playback.
+ *
+ * @author Sam Reid (PhET Interactive Simulations)
+ * @author Michael Kauzmann (PhET Interactive Simulations)
+ */
+define( require => {
+  'use strict';
+
+  // modules
+  const axon = require( 'AXON/axon' );
+  const EmitterIO = require( 'AXON/EmitterIO' );
+  const PhetioObject = require( 'TANDEM/PhetioObject' );
+  const Tandem = require( 'TANDEM/Tandem' );
+  const ValidatorDef = require( 'AXON/ValidatorDef' );
+  const validate = require( 'AXON/validate' );
+
+  // constants
+  const ActionIOWithNoArgs = EmitterIO( [] );
+
+  // Simulations have thousands of Emitters, so we re-use objects where possible.
+  const EMPTY_ARRAY = [];
+  assert && Object.freeze( EMPTY_ARRAY );
+
+  class Action extends PhetioObject {
+
+    /**
+     * @param {function|null} action - the function that is called when this Action occurs
+     * @param {Object} [options]
+     */
+    constructor( action, options ) {
+
+      const phetioTypeSupplied = options && options.hasOwnProperty( 'phetioType' );
+      const validatorsSupplied = options && options.hasOwnProperty( 'validators' );
+
+      // if ( assert && phetioTypeSupplied ) {
+      //   assert( options.phetioType.parameterTypes.length > 0, 'do not specify phetioType that is the same as the default' );
+      // }
+
+      options = _.extend( {
+
+        // {Array.<Object>|null} - array of "validators" as defined by ValidatorDef.js
+        validators: EMPTY_ARRAY,
+
+        // {boolean} @deprecated, only to support legacy emit1, emit2, emit3 calls.
+        validationEnabled: true,
+
+        // phet-io
+        tandem: Tandem.optional,
+        phetioState: false,
+        phetioType: ActionIOWithNoArgs // subtypes can override with EmitterIO([...]), see EmitterIO.js
+      }, options );
+
+      assert && assert( !options.hasOwnProperty( 'listener' ), 'listener option no longer supported, please use first' );
+      assert && assert( !options.hasOwnProperty( 'before' ), 'before option no longer supported, please use first' );
+
+      // phetioPlayback events need to know the order the arguments occur in order to call EmitterIO.emit()
+      // Indicate whether the event is for playback, but leave this "sparse"--only indicate when this happens to be true
+      if ( options.phetioPlayback ) {
+        options.phetioEventMetadata = options.phetioEventMetadata || {};
+        assert && assert( !options.phetioEventMetadata.hasOwnProperty( 'dataKeys' ), 'dataKeys should be supplied by Emitter, not elsewhere' );
+        options.phetioEventMetadata.dataKeys = options.phetioType.elements.map( element => element.name );
+      }
+
+      // important to be before super call. OK to supply neither or one or the other, but not both.  That is a NAND.
+      assert && assert( !( phetioTypeSupplied && validatorsSupplied ),
+        'use either phetioType or validators, not both, see EmitterIO to set validators on an instrumented Action'
+      );
+
+      // use the phetioType's validators if provided, we know we aren't overwriting here because of the above assertion
+      if ( phetioTypeSupplied ) {
+        options.validators = options.phetioType.validators;
+      }
+
+      super( options );
+
+      validate( options.validators, { valueType: Array } );
+
+      // @public (only for testing) - Note: one test indicates stripping this out via assert && in builds may save around 300kb heap
+      this.validators = options.validators;
+
+      // @private - opt out of validation. Can be removed when deprecated emit functions are gone.
+      this.validationEnabled = options.validationEnabled;
+
+      if ( assert ) {
+
+        // Iterate through each validator and make sure that it won't validate options on validating value. This is
+        // mainly done for performance
+        options.validators.forEach( validator => {
+          assert && assert(
+            validator.validateOptionsOnValidateValue !== true,
+            'emitter sets its own validateOptionsOnValidateValue for each argument type'
+          );
+          validator.validateOptionsOnValidateValue = false;
+
+          // Changing the validator options after construction indicates a logic error, except that many EmitterIOs
+          // are shared between instances. Don't assume we "own" the validator if it came from the TypeIO.
+          assert && !phetioTypeSupplied && Object.freeze( validator );
+
+          // validate the options passed in to validate each emitter argument
+          assert && ValidatorDef.validateValidator( validator );
+        } );
+
+        // Changing after construction indicates a logic error, except that many EmitterIOs are shared between instances.
+        // Don't assume we "own" the validator if it came from the TypeIO.
+        assert && !phetioTypeSupplied && Object.freeze( options.validators );
+      }
+
+      // @protected - can be supplied by subclasses after superconstructor call completes
+      this.action = action;
+    }
+
+    /**
+     * Gets the data that will be emitted to the PhET-iO data stream, for an instrumented simulation.
+     * @returns {*}
+     * @private
+     */
+    getPhetioData() {
+
+      // null if there are no arguments.  dataStream.js omits null values for data
+      let data = null;
+      if ( this.phetioType.elements.length > 0 ) {
+
+        // Enumerate named argsObject for the data stream.
+        data = {};
+        for ( let i = 0; i < this.phetioType.elements.length; i++ ) {
+          const element = this.phetioType.elements[ i ];
+          data[ element.name ] = element.type.toStateObject( arguments[ i ] );
+        }
+      }
+      return data;
+    }
+
+    /**
+     * Emits a single event.  This method is called many times in a simulation and must be well-optimized.  Listeners
+     * are notified in the order they were added via addListener, though it is poor practice to rely on the order
+     * of listener notifications.
+     * @params - expected parameters are based on options.validators, see constructor
+     * @public
+     */
+    emit() {
+      assert && assert( typeof this.action === 'function', 'action should exist when emit is called' );
+      if ( assert && this.validationEnabled ) {
+        assert(
+          arguments.length === this.validators.length,
+          `Emitted unexpected number of args. Expected: ${this.validators.length} and received ${arguments.length}`
+        );
+        for ( let i = 0; i < this.validators.length; i++ ) {
+          validate( arguments[ i ], this.validators[ i ] );
+        }
+      }
+
+      // handle phet-io data stream for the emitted event
+      this.isPhetioInstrumented() && this.phetioStartEvent( 'emitted', this.getPhetioData.apply( this, arguments ) );
+
+      this.action.apply( null, arguments );
+
+      this.isPhetioInstrumented() && this.phetioEndEvent();
+    }
+  }
+
+  return axon.register( 'Action', Action );
+} );
\ No newline at end of file
Index: js/Emitter.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/Emitter.js	(revision 2da6c6173a0ed6bedc1dad8a5fffac27faaf5259)
+++ js/Emitter.js	(date 1554395118000)
@@ -9,110 +9,54 @@
   'use strict';
 
   // modules
+  const Action = require( 'AXON/Action' );
   const axon = require( 'AXON/axon' );
   const EmitterIO = require( 'AXON/EmitterIO' );
-  const PhetioObject = require( 'TANDEM/PhetioObject' );
-  const Tandem = require( 'TANDEM/Tandem' );
-  const ValidatorDef = require( 'AXON/ValidatorDef' );
-  const validate = require( 'AXON/validate' );
 
   // constants
-  const EmitterIOWithNoArgs = EmitterIO( [] );
+  const EmitterIOWithNoArgs = EmitterIO( [] ); // TODO:
 
   // Simulations have thousands of Emitters, so we re-use objects where possible.
   const EMPTY_ARRAY = [];
   assert && Object.freeze( EMPTY_ARRAY );
 
-  /**
-   * @param {Object} [options]
-   */
-  class Emitter extends PhetioObject {
+  // TODO: create LightweightEmitter which does not extend PhetioObject, but used here by composition
+
+  // horrible
+  // var myEmitter = isPhetio? new PhetioEmitter() : new RegularEmitter();
+
+  class Emitter extends Action {
+
+    /**
+     * @param {Object} [options]
+     */
     constructor( options ) {
 
-      const phetioTypeSupplied = options && options.hasOwnProperty( 'phetioType' );
-      const validatorsSupplied = options && options.hasOwnProperty( 'validators' );
-
-      if ( assert && phetioTypeSupplied ) {
-        assert( options.phetioType.parameterTypes.length > 0, 'do not specify phetioType that is the same as the default' );
-      }
+      // options = _.extend( {
+      //
+      //   // phetioType: EmitterIOWithNoArgs // subtypes can override with EmitterIO([...]), see EmitterIO.js
+      // }, options );
 
-      options = _.extend( {
+      super( null, options );
 
-        // {Array.<Object>|null} - array of "validators" as defined by ValidatorDef.js
-        validators: EMPTY_ARRAY,
+      const self = this;
 
-        // {boolean} @deprecated, only to support legacy emit1, emit2, emit3 calls.
-        validationEnabled: true,
+      // Set the action in the parent type now that we have self
+      this.action = function() {
 
-        // {function|null} [first] optional listener which will be added as the first listener.
-        // Can be removed via removeListener.
-        first: null,
+        // Notify wired-up listeners, if any
+        if ( self.listeners.length > 0 ) {
+          self.activeListenersStack.push( self.listeners );
 
-        // {function|null} [last] optional listener which will be added as the last listener.
-        // Can be removed via removeListener.
-        last: null,
-
-        // phet-io
-        tandem: Tandem.optional,
-        phetioState: false,
-        phetioType: EmitterIOWithNoArgs // subtypes can override with EmitterIO([...]), see EmitterIO.js
-
-      }, options );
-
-      assert && assert( !options.hasOwnProperty( 'listener' ), 'listener option no longer supported, please use first' );
-      assert && assert( !options.hasOwnProperty( 'before' ), 'before option no longer supported, please use first' );
-
-      // phetioPlayback events need to know the order the arguments occur in order to call EmitterIO.emit()
-      // Indicate whether the event is for playback, but leave this "sparse"--only indicate when this happens to be true
-      if ( options.phetioPlayback ) {
-        options.phetioEventMetadata = options.phetioEventMetadata || {};
-        assert && assert( !options.phetioEventMetadata.hasOwnProperty( 'dataKeys' ), 'dataKeys should be supplied by Emitter, not elsewhere' );
-        options.phetioEventMetadata.dataKeys = options.phetioType.elements.map( element => element.name );
-      }
+          // Notify listeners--note the activeListenersStack could change as listeners are called, so we do this by index
+          const lastEntry = self.activeListenersStack.length - 1;
+          for ( let i = 0; i < self.activeListenersStack[ lastEntry ].length; i++ ) {
+            self.activeListenersStack[ lastEntry ][ i ].apply( null, arguments );
+          }
 
-      // important to be before super call. OK to supply neither or one or the other, but not both.  That is a NAND.
-      assert && assert( !( phetioTypeSupplied && validatorsSupplied ),
-        'use either phetioType or validators, not both, see EmitterIO to set validators on an instrumented Emitter'
-      );
-
-      // use the phetioType's validators if provided, we know we aren't overwriting here because of the above assertion
-      if ( phetioTypeSupplied ) {
-        options.validators = options.phetioType.validators;
-      }
-
-      super( options );
-
-      validate( options.validators, { valueType: Array } );
-
-      // @public (only for testing) - Note: one test indicates stripping this out via assert && in builds may save around 300kb heap
-      this.validators = options.validators;
-
-      // @private - opt out of validation. Can be removed when deprecated emit functions are gone.
-      this.validationEnabled = options.validationEnabled;
-
-      if ( assert ) {
-
-        // Iterate through each validator and make sure that it won't validate options on validating value. This is
-        // mainly done for performance
-        options.validators.forEach( validator => {
-          assert && assert(
-            validator.validateOptionsOnValidateValue !== true,
-            'emitter sets its own validateOptionsOnValidateValue for each argument type'
-          );
-          validator.validateOptionsOnValidateValue = false;
-
-          // Changing the validator options after construction indicates a logic error, except that many EmitterIOs
-          // are shared between instances. Don't assume we "own" the validator if it came from the TypeIO.
-          assert && !phetioTypeSupplied && Object.freeze( validator );
-
-          // validate the options passed in to validate each emitter argument
-          assert && ValidatorDef.validateValidator( validator );
-        } );
-
-        // Changing after construction indicates a logic error, except that many EmitterIOs are shared between instances.
-        // Don't assume we "own" the validator if it came from the TypeIO.
-        assert && !phetioTypeSupplied && Object.freeze( options.validators );
-      }
+          self.activeListenersStack.pop();
+        }
+      };
 
       // @private {function[]} - the listeners that will be called on emit
       this.listeners = [];
@@ -120,14 +64,6 @@
       // @private {function[][]} - during emit() keep track of which listeners should receive events in order to manage
       //                         - removal of listeners during emit()
       this.activeListenersStack = [];
-
-      // @private {function|null} if defined, called as the first listener
-      this.first = options.first;
-      this.first && this.listeners.push( this.first );
-
-      // @private {function|null} if defined, called as the last listener
-      this.last = options.last;
-      this.last && this.listeners.push( this.last );
     }
 
     /**
@@ -136,8 +72,6 @@
      * @override
      */
     dispose() {
-      this.first = null;
-      this.last = null;
       this.listeners.length = 0; // See https://github.com/phetsims/axon/issues/124
       super.dispose();
     }
@@ -180,14 +114,6 @@
       this.defendListeners();
 
       this.listeners.splice( index, 1 );
-
-      // Cleanup for special cases of first and last
-      if ( this.last === listener ) {
-        this.last = null;
-      }
-      if ( this.first === listener ) {
-        this.first = null;
-      }
     }
 
     /**
@@ -225,67 +151,6 @@
       }
     }
 
-    /**
-     * Gets the data that will be emitted to the PhET-iO data stream, for an instrumented simulation.
-     * @returns {*}
-     * @private
-     */
-    getPhetioData() {
-
-      // null if there are no arguments.  dataStream.js omits null values for data
-      let data = null;
-      if ( this.phetioType.elements.length > 0 ) {
-
-        // Enumerate named argsObject for the data stream.
-        data = {};
-        for ( let i = 0; i < this.phetioType.elements.length; i++ ) {
-          const element = this.phetioType.elements[ i ];
-          data[ element.name ] = element.type.toStateObject( arguments[ i ] );
-        }
-      }
-      return data;
-    }
-
-    /**
-     * Emits a single event.  This method is called many times in a simulation and must be well-optimized.  Listeners
-     * are notified in the order they were added via addListener, though it is poor practice to rely on the order
-     * of listener notifications.
-     * @params - expected parameters are based on options.validators, see constructor
-     * @public
-     */
-    emit() {
-      if ( assert && this.validationEnabled ) {
-        assert(
-          arguments.length === this.validators.length,
-          `Emitted unexpected number of args. Expected: ${this.validators.length} and received ${arguments.length}`
-        );
-        for ( let i = 0; i < this.validators.length; i++ ) {
-          validate( arguments[ i ], this.validators[ i ] );
-        }
-      }
-
-      assert && this.first && assert( this.listeners.indexOf( this.first ) === 0, 'first should be at the beginning' );
-      assert && this.last && assert( this.listeners.indexOf( this.last ) === this.listeners.length - 1, 'last should be ' +
-                                                                                                        'at the end' );
-      // handle phet-io data stream for the emitted event
-      this.isPhetioInstrumented() && this.phetioStartEvent( 'emitted', this.getPhetioData.apply( this, arguments ) );
-
-      // Notify wired-up listeners, if any
-      if ( this.listeners.length > 0 ) {
-        this.activeListenersStack.push( this.listeners );
-
-        // Notify listeners--note the activeListenersStack could change as listeners are called, so we do this by index
-        const lastEntry = this.activeListenersStack.length - 1;
-        for ( let i = 0; i < this.activeListenersStack[ lastEntry ].length; i++ ) {
-          this.activeListenersStack[ lastEntry ][ i ].apply( null, arguments );
-        }
-
-        this.activeListenersStack.pop();
-      }
-
-      this.isPhetioInstrumented() && this.phetioEndEvent();
-    }
-
     /**
      * Checks whether a listener is registered with this Emitter
      * @param {function} listener

[pixelzoom]

4/4/19 meeting notes, @samreid @zepumph @pixelzoom

We quickly decided to proceed with what @samreid proposed in #222 (comment):

As a first pass, I'd recommend investigating renaming Emitter to Task and removing the listener functionality. Then creating a new subclass of Task called Emitter which adds listeners.

Then @samreid and @zepumph started pairing, while I went back to work on Gas Properties. @samreid noted their progress in #222 (comment).

[zepumph]

If we decide that TaskIO becomes the main building block of the event stream. This will definitely complicate backwards compatibility. Let's try to get this done before the first stable release.

[pixelzoom]

Self unassigning. Please assign back to me when there's something to review, or when we're ready to move to the next step in the checklist ("discuss with the dev team").

[samreid]

Expanding on #222 (comment) I'm close to a commit point.

What was done

• Introduce Action.js
• Use Action instead of Emitter.first/last

What's working:

• Axon unit tests pass
• Sims are running, time steps happen and input events are processed
• Mirror inputs is working
• Lint is passing

Questions or remaining steps

• Create TinyEmitter and use by composition? Or is that too heavyweight to introduce a new object for every Emitter? Graphing Quadratics instantiates 6000 Emitters on startup. Or maybe there is a better way to combine Action and TinyEmitter?
• Create ActionIO? Or will that be too complex to have one parametric IO type subtype another parametric IO type?
• see Rename Action, instances of {Action}, and Action.emit() #243 Everything converted to Action is still named as Emitter
• Full aqua testing
• seehttps://github.com/Rename Action, instances of {Action}, and Action.emit() #243 Rename Action? Rename Action.emit?
• Some TODO investigate factoring out PhET-iO data stream responsibilities from Emitter #222 marked in the code

UPDATE: Aqua is coming out green, so I'll commit to master and we can take next steps there.

UPDATE: Notifying slack:

Major changes about to be committed for #222 (comment), which factors a base type Action out of Emitter into a new base type Action. Changes affect joist, axon, scenery, sun and more. Local testing coverage is described in #222 (comment) but please pull and report any problems you discover.

[zepumph]

Emitter constructor: This seems unnecessary, or possibly misplaced, since it's duplicated in Action.

This is trying to explain both checks, in Emitter and Action. Unfortunately it is necessary at least for the granularity of assertion we are looking for. The reason is that we want to be able to assert out if ever a phetioType passes in an ActionIO (or subtype) with no args. In Emitter.js right after this check we set the phetioType set to the no arg default. If we waited to check this in the super type, by the time we get over to Action, we wouldn't know if the EmitterIO with no args came from the default in Emitter.js, or if it was provided by the client. We ended up handling the EmitterIO case here, and the ActionIO case in ActionIO. It isn't the cleanest solution, but @samreid and I like that it kept the assertions as strict as they were prior to this refactoring. @samreid please let me know if you feel differently or have other recommendations.

Emitter constructor: const self = this is unnecessary, use arrow function for this.action.

We found this to be necessary, because arrow functions don't support arguments keyword.

constructor: options logic before super is complicated, looks difficult to maintain. Can this be clarified or simplified?

I worked for awhile trying to make this simpler and easier. I pulled out lots of the validation to a separate function responsible for validating the validators. I moved assertions around to where I think they make more sense. I added more documentation. @samreid would you please review this piece in particular. I would like to have your eyes here to see if things could be more maintainable.

constructor: I don't understand the comment here. Why is super relevant, and why should subclasses be able to assign to this? (implied by "supplied")

and

emit: this assertion belongs in constructor:

To solve these two, I did a slight amount of hackary, in which Emitter.js now specifies a function to pass to the super call BEFORE elements called within the function are defined, for example self.tinyEmitter where self and tinyEmitter are defined after the super call. This is weird to read, but it turned out to be really nice for Action, because we could then remove the {null|function} type for the parameter, and enforce that the "action" function is passed in to the constructor.

So this has the potential to break clients that use logic based on isDisposed

TinyEmitter's constructor is pretty simple, and the isDisposed field is clearly marked private. I wonder if that is enough? I looked through all usages of .isDisposed and didn't see any for Emitters, and only a handful for Properties. Most were for Nodes. Seems like a safe enough discrepancy to me.

I went through all of the checkboxes and completed them or pulled them out to separate issues. @samreid would you please take a look at my comments above, and review the pieces that mention you. I think we are safe to take this off of "blocking non phet-io publication." Let me know if you feel differently. Also feel free to remove this from blocking phet-io publication if you think it should be.

What are the next steps for this issue?

[samreid]

We ended up handling the EmitterIO case here, and the ActionIO case in ActionIO. It isn't the cleanest solution, but @samreid and I like that it kept the assertions as strict as they were prior to this refactoring. @samreid please let me know if you feel differently or have other recommendations.

It's appropriate to have the specialized check for 0-parameter ActionIO in the base type and another check for 0-parameter EmitterIO in the subtype.

@samreid would you please review this piece in particular. I would like to have your eyes here to see if things could be more maintainable.

The code has:

      // Extend options with values set by Action, only if there are any to set to save an object declaration.
      if ( Object.keys( optionsSetByAction ).length > 0 ) {
        options = _.extend( {}, options, optionsSetByAction );
      }

Object.keys() allocates an array, which is contrary to the stated goal of avoid allocations.

Also there is this part:

      options = _.extend( {

        // {ValidatorDef[]}
        validators: EMPTY_ARRAY,

        // {boolean} @deprecated, only to support legacy emit1, emit2, emit3 calls.
        validationEnabled: true,

        // phet-io - see PhetioObject.js for doc
        tandem: Tandem.optional,
        phetioState: false,
        phetioType: ActionIOWithNoArgs, // subtypes can override with ActionIO([...]), see ActionIO.js
        phetioPlayback: PhetioObject.DEFAULT_OPTIONS.phetioPlayback,
        phetioEventMetadata: PhetioObject.DEFAULT_OPTIONS.phetioEventMetadata
      }, options );

      // Use a separate object so as to not mutate passed in options object.
      const optionsSetByAction = {};

In this part, the extend call returns a new object, so subsequent calls cannot mutate the passed-in options object. Therefore, it seems we can eliminate optionsSetByAction and set values directly on options. This will be simpler and more consistent with options manipulation in other files.

@zepumph can you please look into those ideas? After that, it seems good to run this by @pixelzoom again.

[zepumph]

@samreid I removed optionsSetByAction above.

I think that the way it is now is not the "best practices" since that dictates two extend calls, but I'm alright doing it this way for such a fundamental type where performance is a concern.

Label for dev meeting when we have something concrete to discuss with the dev team.

@pixelzoom this checkbox from the first comment is all that I can think of that remains, besides if you have anything else here. Perhaps the naming discussion in #243 was enough? Over to you for your opinion. Are we ready to close?

[pixelzoom]

• Label for dev meeting when we have something concrete to discuss with the dev team.

I believe the intent of this item was to present/summarize the results at a dev meeting. That hasn't been done, so no, not ready to close this issue.

[zepumph]

Label for dev meeting when we have something concrete to discuss with the dev team.

Perhaps originally we meant to do this before implementing the entire thing, but instead we have implemented a (potentially) complete solution that we now want to communicate to other devs both as a PSA and place for feedback.

Summary of work done here and supporting issues:

• Emitter's complexities and responsibilities have been divided into three files:
  • Action - base type to Emitter, responsible for executing an action, validating the arguments passed to that action, and emitting that action to the PhET-iO data stream if the Action is instrumented (by extending PhetioObject). Action.js uses the execute function instead of emit
  • TinyEmitter - The lightest-weight notification system possible. Extends Object, and is meant to be used in places where performance or low heap usage is very important.
  • Emitter - Subtype of Action, adding the listener logic and the emit method calls execute. Emitter uses TinyEmitter via composition to support listeners and notifications to them.
• We changed phet-io usages of Emitter over to Action where appropriate, for example in Input.js all Emitters there just added a single "listener" to be called on the specific input event.

@samreid @pixelzoom @chrisklus please add to this if you feel like there is more. Assigning to developer meeting.

[samreid]

We reviewed and discussed, closing.