Cleanup regarding Emitter argumentTypes and usage, see #182

phetsims · Jan 16, 2019 · 52c9a17 · 52c9a17
1 parent 4dcf357
commit 52c9a17
Show file tree

Hide file tree

Showing 2 changed files with 37 additions and 27 deletions.
diff --git a/js/Emitter.js b/js/Emitter.js
@@ -18,6 +18,10 @@ define( require => {
   // constants
   const EmitterIOWithNoArgs = EmitterIO( [] );
 
+  // Simulations have thousands of Emitters, so we re-use objects where possible.
+  const EMPTY_ARRAY = [];
+  assert && Object.freeze( EMPTY_ARRAY );
+
   /**
    * @param {Object} [options]
    */
@@ -28,7 +32,7 @@ define( require => {
 
         // {Array.<Object>|null} - array of "Validator Options" Objects that hold options for how to validate each
         // argument, see Validator.js for details.
-        argumentTypes: null,
+        argumentTypes: EMPTY_ARRAY,
 
         tandem: Tandem.optional,
         phetioState: false,
@@ -46,38 +50,37 @@ define( require => {
 
       super( options );
 
-      // TODO: perhaps this should be the default, but not until we switch all usages, see https://github.com/phetsims/axon/issues/182
-      if ( !options.argumentTypes ) {
-        options.argumentTypes = [];
-      }
-
-      assert && assert( Array.isArray( options.argumentTypes ),
-        `incorrect type for argumentTypes: ${typeof options.argumentTypes}` );
-
-
-      // @private
-      this.numberOfArgs = options.argumentTypes.length;
+      Validator.validate( options.argumentTypes, { valueType: Array } );
 
       if ( assert ) {
 
         // Iterate through all argumentType validator options and make sure that they won't validate options on validating value
-        for ( let i = 0; i < options.argumentTypes.length; i++ ) {
-          const validatorOptions = options.argumentTypes[ i ];
-          assert && assert( validatorOptions.validateOptionsOnValidateValue === undefined, 'emitter sets its own validateOptionsOnValidateValue for each argument type' );
+        options.argumentTypes.forEach( validatorOptions => {
+          assert && assert(
+            validatorOptions.validateOptionsOnValidateValue === undefined,
+            'emitter sets its own validateOptionsOnValidateValue for each argument type'
+          );
           validatorOptions.validateOptionsOnValidateValue = false;
 
+          // Changing the validator options after construction indicates a logic error
+          assert && Object.freeze( validatorOptions );
+
           // validate the options passed in to validate each emitter argument
           Validator.validateOptions( validatorOptions );
-        }
+        } );
+
+        // Changing after construction indicates a logic error
+        assert && Object.freeze( options.argumentTypes );
       }
 
-      //@private
-      this.assertEmittingValidValues = assert && function() {
-        const args = arguments;
-        assert( args.length === this.numberOfArgs,
-          `Emitted unexpected number of args. Expected: ${this.numberOfArgs} and received ${args.length}` );
+      // @private {function|false}
+      this.validate = assert && function() {
+        assert(
+          arguments.length === options.argumentTypes.length,
+          `Emitted unexpected number of args. Expected: ${options.argumentTypes.length} and received ${arguments.length}`
+        );
         for ( let i = 0; i < options.argumentTypes.length; i++ ) {
-          Validator.validate( args[ i ], options.argumentTypes[ i ] );
+          Validator.validate( arguments[ i ], options.argumentTypes[ i ] );
         }
       };
 
@@ -187,7 +190,7 @@ define( require => {
     emit() {
 
       // validate the args
-      this.assertEmittingValidValues && this.assertEmittingValidValues.apply( this, arguments );
+      this.validate && this.validate.apply( this, arguments );
 
       // handle phet-io data stream for the emitted event
       if ( this.isPhetioInstrumented() ) {

diff --git a/js/EmitterTests.js b/js/EmitterTests.js
@@ -39,21 +39,29 @@ define( require => {
 
     e2.emit( new Emitter(), {}, () => {} );
 
-    const type = v => v === null || typeof v === 'string';
+    if ( window.assert ) {
+      assert.throws( () => { e2.emit( 2, 2 ); }, 'Wrong number of emitting parameters, for e2' );
+      assert.throws( () => { e2.emit( true ); }, 'Wrong parameter type bool, for e2' );
+      assert.throws( () => { e2.emit( '2, 2' ); }, 'Wrong parameter type string, for e2' );
+      assert.throws( () => { e2.emit( undefined ); }, 'Wrong parameter type undefined, for e2' );
+      assert.throws( () => { e2.emit( null ); }, 'Wrong parameter type null, for e2' );
+      assert.throws( () => { e2.emit( new Emitter(), 7, () => {} ); }, 'Should catch second argument as wrong type' );
+      assert.throws( () => { e2.emit( new Emitter() ); }, 'Should catch not enough arguments' );
+    }
 
     const e3 = new Emitter( {
-      argumentTypes: [ { valueType: 'number' }, { isValidValue: type } ]
+      argumentTypes: [ { valueType: 'number' }, { isValidValue: v => v === null || typeof v === 'string' } ]
     } );
 
     e3.emit( 1, 'hi' );
     e3.emit( 1, null );
+
     if ( window.assert ) {
       assert.throws( () => { e3.emit( 1 ); }, 'Wrong parameter type undefined' );
       assert.throws( () => { e3.emit( 1, undefined ); }, 'Wrong parameter type undefined' );
       assert.throws( () => { e3.emit( 1, 0 ); }, 'Wrong parameter type 0' );
       assert.throws( () => { e3.emit( 1, { hello: 'hi' } ); }, 'Wrong parameter type object' );
     }
-
   } );
 
   QUnit.test( 'Test emit timing Emitter', assert => {
@@ -73,7 +81,6 @@ define( require => {
     const e1 = new Emitter();
     e1.addListener( () => {} );
 
-
     const testEmitter = ( e, numberOfLoopings ) => {
 
       const start = Date.now();