IO Type factory based on methods from the core type?

[zepumph]

Based on patterns that are getting established for IO Types in natural-selection and MultipleParticleModelIO, most of the logic in an IO Type is actually getting defined in the core type, and so IO Types often are mostly boilerplate:

class BunnyIO extends ObjectIO {

  /**
   * Serializes a Bunny instance.
   * @param {Bunny} bunny
   * @returns {Object}
   * @public
   * @override
   */
  static toStateObject( bunny ) {
    validate( bunny, this.validator );
    return bunny.toStateObject();
  }

  /**
   * Deserializes a Bunny instance.
   * @param {Object} stateObject
   * @returns {Object}
   * @public
   * @override
   */
  static fromStateObject( stateObject ) {
    return Bunny.fromStateObject( stateObject );
  }

  /**
   * Creates the args to BunnyGroup.createNextElement that creates Bunny instances.
   * @param state
   * @returns {Object[]}
   * @public
   * @override
   */
  static stateToArgsForConstructor( state ) {
    return Bunny.stateToArgsForConstructor( state );
  }

  /**
   * Restores Bunny state after instantiation.
   * @param {Bunny} bunny
   * @param {Object} state
   * @public
   * @override
   */
  static applyState( bunny, state ) {
    validate( bunny, this.validator );
    bunny.applyState( state );
  }
}

In those cases, it could be nice to have an IO Type factory that can just wrap the core type.

[samreid]

It seems like we should coordinate this work with the other IO type work proposed in

https://github.com/phetsims/phet-io/issues/1657#issuecomment-646735445
https://github.com/phetsims/phet-io/issues/1646
https://github.com/phetsims/phet-io/issues/1643
https://github.com/phetsims/phet-io/issues/1657#issuecomment-646735445

Do we need an IO type label?

Unassigning and adding to Wednesday agenda.

[samreid]

In https://github.com/phetsims/phet-io/issues/1683#issuecomment-646229929 @pixelzoom said:

In other issues we have decided that, by convention, many functions of the IO Type will just forward a method over to the core type/instance, I.E. BunnyIO.toStateObject calls Bunny.prototyp.toStateObject.

The reason for this convention is that serialization/deserialization typically involves accessing private fields of the core type. If the IO Type accesses those fields, it's violating the core type's API. If the IO Type delegates to the core type, then it's not a problem.

[samreid]

Here's a patch that introduces a new function createIOType and also demonstrates its use for BunnyIO. The usage site looks like this:

createIOType( Bunny, 'BunnyIO', 'IO Type for Bunny', {} )

All validation & forwarding is set up automatically. If there are any methods, they can be supplied in the options.

Additionally, if we declare it within the during the declaration for Bunny.js, we can have direct type checks without using the namespace and without any circular module loading problems:

Bunny.BunnyIO = createIOType( Bunny, 'BunnyIO', 'IO Type for Bunny', {} )

I'd like to get feedback from @pixelzoom and/or @zepumph before moving more in this direction, but it looks promising to me.

Index: main/natural-selection/js/common/model/BunnyIO.js
===================================================================
--- main/natural-selection/js/common/model/BunnyIO.js	(revision 9bb444d45c9657de620916ab4162c9874ce1c973)
+++ main/natural-selection/js/common/model/BunnyIO.js	(revision 9bb444d45c9657de620916ab4162c9874ce1c973)
@@ -1,58 +0,0 @@
-// Copyright 2020, University of Colorado Boulder
-
-/**
- * BunnyIO is the IO Type for Bunny. It delegates most of its implementation to Bunny.
- *
- * @author Chris Malley (PixelZoom, Inc.)
- */
-
-import validate from '../../../../axon/js/validate.js';
-import ObjectIO from '../../../../tandem/js/types/ObjectIO.js';
-import naturalSelection from '../../naturalSelection.js';
-import Bunny from './Bunny.js';
-
-class BunnyIO extends ObjectIO {
-
-  /**
-   * Serializes a Bunny instance.
-   * @param {Bunny} bunny
-   * @returns {Object}
-   * @public
-   * @override
-   */
-  static toStateObject( bunny ) {
-    validate( bunny, this.validator );
-    return bunny.toStateObject();
-  }
-
-  /**
-   * Creates the args to BunnyGroup.createNextElement that creates Bunny instances.
-   * @param state
-   * @returns {Object[]}
-   * @public
-   * @override
-   */
-  static stateToArgsForConstructor( state ) {
-    return Bunny.stateToArgsForConstructor( state );
-  }
-
-  /**
-   * Restores Bunny state after instantiation.
-   * @param {Bunny} bunny
-   * @param {Object} state
-   * @public
-   * @override
-   */
-  static applyState( bunny, state ) {
-    validate( bunny, this.validator );
-    bunny.applyState( state );
-  }
-}
-
-BunnyIO.documentation = 'IO Type for Bunny';
-BunnyIO.validator = { isValidValue: value => value instanceof Bunny };
-BunnyIO.typeName = 'BunnyIO';
-ObjectIO.validateSubtype( BunnyIO );
-
-naturalSelection.register( 'BunnyIO', BunnyIO );
-export default BunnyIO;
\ No newline at end of file
Index: main/natural-selection/js/common/model/BunnyGroup.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- main/natural-selection/js/common/model/BunnyGroup.js	(revision 9bb444d45c9657de620916ab4162c9874ce1c973)
+++ main/natural-selection/js/common/model/BunnyGroup.js	(date 1598296691115)
@@ -15,7 +15,7 @@
 import Tandem from '../../../../tandem/js/Tandem.js';
 import naturalSelection from '../../naturalSelection.js';
 import Bunny from './Bunny.js';
-import BunnyIO from './BunnyIO.js';
+// import BunnyIO from './BunnyIO.js';
 import EnvironmentModelViewTransform from './EnvironmentModelViewTransform.js';
 import GenePool from './GenePool.js';
 
@@ -37,7 +37,7 @@
 
       // phet-io
       tandem: Tandem.REQUIRED,
-      phetioType: PhetioGroupIO( BunnyIO ),
+      phetioType: PhetioGroupIO( Bunny.BunnyIO ),
       phetioDocumentation: 'manages dynamic PhET-iO elements of type Bunny, including live and dead bunnies'
     }, options );
 
Index: main/natural-selection/js/common/model/BunnyArray.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- main/natural-selection/js/common/model/BunnyArray.js	(revision 9bb444d45c9657de620916ab4162c9874ce1c973)
+++ main/natural-selection/js/common/model/BunnyArray.js	(date 1598297648845)
@@ -13,9 +13,9 @@
 import Tandem from '../../../../tandem/js/Tandem.js';
 import ReferenceIO from '../../../../tandem/js/types/ReferenceIO.js';
 import naturalSelection from '../../naturalSelection.js';
+import Bunny from './Bunny.js';
 import BunnyCounts from './BunnyCounts.js';
 import BunnyCountsIO from './BunnyCountsIO.js';
-import BunnyIO from './BunnyIO.js';
 
 class BunnyArray extends AxonArray {
 
@@ -34,7 +34,7 @@
 
       // phet-io
       tandem: Tandem.REQUIRED,
-      phetioElementType: ReferenceIO( BunnyIO ),
+      phetioElementType: ReferenceIO( Bunny.BunnyIO ),
       phetioState: false
     }, options );
 
Index: main/natural-selection/js/common/model/Bunny.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- main/natural-selection/js/common/model/Bunny.js	(revision 9bb444d45c9657de620916ab4162c9874ce1c973)
+++ main/natural-selection/js/common/model/Bunny.js	(date 1598296717518)
@@ -17,12 +17,12 @@
 import AssertUtils from '../../../../phetcommon/js/AssertUtils.js';
 import Tandem from '../../../../tandem/js/Tandem.js';
 import BooleanIO from '../../../../tandem/js/types/BooleanIO.js';
+import createIOType from '../../../../tandem/js/types/createIOType.js';
 import NullableIO from '../../../../tandem/js/types/NullableIO.js';
 import NumberIO from '../../../../tandem/js/types/NumberIO.js';
 import ReferenceIO from '../../../../tandem/js/types/ReferenceIO.js';
 import naturalSelection from '../../naturalSelection.js';
 import NaturalSelectionUtils from '../NaturalSelectionUtils.js';
-import BunnyIO from './BunnyIO.js';
 import EnvironmentModelViewTransform from './EnvironmentModelViewTransform.js';
 import GenePool from './GenePool.js';
 import Genotype from './Genotype.js';
@@ -48,6 +48,8 @@
    */
   constructor( genePool, modelViewTransform, bunnyRestRangeProperty, options ) {
 
+    console.log( Bunny );
+
     assert && assert( genePool instanceof GenePool, 'invalid genePool' );
     assert && assert( modelViewTransform instanceof EnvironmentModelViewTransform, 'invalid modelViewTransform' );
     assert && AssertUtils.assertPropertyOf( bunnyRestRangeProperty, Range );
@@ -431,5 +433,8 @@
   return new Vector3( dx, dy, dz );
 }
 
+const BunnyIO = createIOType( Bunny, 'BunnyIO', 'IO Type for Bunny', {} );
+Bunny.BunnyIO = BunnyIO;
+
 naturalSelection.register( 'Bunny', Bunny );
 export default Bunny;
\ No newline at end of file
Index: main/tandem/js/types/createIOType.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- main/tandem/js/types/createIOType.js	(date 1598297605863)
+++ main/tandem/js/types/createIOType.js	(date 1598297605863)
@@ -0,0 +1,61 @@
+// Copyright 2020, University of Colorado Boulder
+
+import validate from '../../../axon/js/validate.js';
+import tandemNamespace from '../tandemNamespace.js';
+import ObjectIO from './ObjectIO.js';
+
+const createIOType = ( CoreType, typeName, documentation, options ) => {
+  class IOType extends ObjectIO {
+
+    /**
+     * @param {PhetioObject} phetioObject
+     * @returns {Object}
+     * @public
+     * @override
+     */
+    static toStateObject( phetioObject ) {
+      validate( phetioObject, this.validator );
+      return phetioObject.toStateObject();
+    }
+
+    // @public
+    static fromStateObject( stateObject ) {
+      return CoreType.fromStateObject( stateObject );
+    }
+
+    /**
+     * @param state
+     * @returns {Object[]}
+     * @public
+     * @override
+     */
+    static stateToArgsForConstructor( state ) {
+      return CoreType.stateToArgsForConstructor( state );
+    }
+
+    /**
+     * Restores CoreType state after instantiation.
+     * @param {PhetioObject} phetioObject
+     * @param {Object} state
+     * @public
+     * @override
+     */
+    static applyState( phetioObject, state ) {
+      validate( phetioObject, this.validator );
+      phetioObject.applyState( state );
+    }
+  }
+
+  IOType.documentation = documentation;
+  IOType.validator = { valueType: CoreType };
+  IOType.typeName = typeName;
+  if ( options.methods ) {
+    IOType.methods = options.methods;
+  }
+  ObjectIO.validateSubtype( IOType );
+
+  return IOType;
+};
+
+tandemNamespace.register( 'createIOType', createIOType );
+export default createIOType;
\ No newline at end of file
Index: main/natural-selection/js/common/model/SelectedBunnyProperty.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- main/natural-selection/js/common/model/SelectedBunnyProperty.js	(revision 9bb444d45c9657de620916ab4162c9874ce1c973)
+++ main/natural-selection/js/common/model/SelectedBunnyProperty.js	(date 1598296691109)
@@ -14,7 +14,7 @@
 import NullableIO from '../../../../tandem/js/types/NullableIO.js';
 import ReferenceIO from '../../../../tandem/js/types/ReferenceIO.js';
 import naturalSelection from '../../naturalSelection.js';
-import BunnyIO from './BunnyIO.js';
+import Bunny from './Bunny.js';
 
 class SelectedBunnyProperty extends Property {
 
@@ -30,7 +30,7 @@
 
       // phet-io
       tandem: Tandem.REQUIRED,
-      phetioType: PropertyIO( NullableIO( ReferenceIO( BunnyIO ) ) ),
+      phetioType: PropertyIO( NullableIO( ReferenceIO( Bunny.BunnyIO ) ) ),
       phetioDocumentation: 'the selected bunny, null if no bunny is selected'
     }, options );
 

[pixelzoom]

I couldn't apply the patch to natural-selection master HEAD for some reason, lots of merge conflicts. But I inspected the code and I like this direction.

A few suggestions RE createIOType:

• Having to import createIOType is inconvenient. Can createIOType be a static method of PhetioObject? Is there any reason to create an IO Type for something that's not a PhetioObject?
• Missing file header comment and JSdoc for signature ( CoreType, typeName, documentation, options ).
• Make options an optional parameter. It's currently required, if ( options.methods ) will fail, and call sites are providing {}.
• options.methods needs to be defined and documented in the default options.
• Move phetioDocumentation parameter to options, with default'IO Type for ${typeName}', so we don't have to repeat "IO Type for Blah" over and over.

[samreid]

I addressed the review comments and committed PhetioObject.createIOType. One recommendation I did not complete is:

• Move phetioDocumentation parameter to options, with default'IO Type for ${typeName}', so we don't have to repeat "IO Type for Blah" over and over.

The typeName is the IO type name, like BunnyIO so this wouldn't work as a default unless we strip off the IO. Also, at a recent meeting we wanted to investigate leveraging core type JSDoc for this documentation, but we haven't investigated that part yet in https://github.com/phetsims/phet-io/issues/1691

This was far enough along that I was comfortable committing it, but one deficiency that will need to be addressed, is that several times in the PhET-iO engine it checks the existence of a method to decide what to do. In this commit, every IO type provides an implementation for all methods, like toStateObject. To address this, we could:

• Only add these methods dynamically if they are defined in the core type (somehow)
• Add another way of checking whether to invoke these methods.
• Invoke the methods and use their return value accordingly.

UPDATE: Also reassigning @pixelzoom in case he wants to review the parts I addressed.

UPDATE 2: I'd like to check in with @zepumph about the bullet points above, unassigning for now.

[pixelzoom]

This is looking good.

Can we move documentation to options and default it to ''? Since IOType.documentation is optional, I don't think it should be a required parameter to createIOType.

I don't fully understand the implications of the remaining phetioEngine "deficiency". Is there a performance impact?
Does it need to be addressed before this can be used in production code?

[samreid]

Thanks, I moved documentation to options and defaulted to ''.

I don't fully understand the implications of the remaining phetioEngine "deficiency". Is there a performance impact?
Does it need to be addressed before this can be used in production code?

There are several places where we check the existence of a method to determine how to proceed. For instance, phetioCommandProcessor.js

    // Serialize the result if possible.
    return methodSignature.returnType.toStateObject ?
           methodSignature.returnType.toStateObject( result ) :
           result;

With PhetioObject.createIOType as it is, toStateObject will always be defined. There are a few other methods and call sites like this. It must be addressed before production usage.

I think we should move toward a pattern where toStateObject is always defined, but some implementations will just return result.

[zepumph]

Beginning of review:

• If we really want this to be the end all, we need to support:

  • Super types (@samreid hopes for no supertypes
  • parametric types

• We need to make sure that we aren't sacrificing the value of IO types as public schema and documentation in exchange for an easier time coding them up. @samreid and I discussed a few places where this is currently happening. If we don't support inheritance, how will methods or constructor logic be transferred instead of duplicated (like for DerivedPropertyIO taking the link method from PropertyIO. We also mentioned how transferring all logic onto the core type is great in many cases, but doesn't address the potential need for the IO type hierarchy to deviate from the core type hierarchy.

• In regards to the toStateObject ?toStateObject( result ): null check. We discussed and think that is out of date. ObjectIO now defines all these methods, and that chunk can now be written like:

    const args = command.args.map( ( arg, index ) => {
      const TypeIO = methodSignature.parameterTypes[ index ];
      assert && assert( !( typeof arg === 'object' && TypeIO.wrapForPhetioCommandProcessor ), 'unsupported arg TypeIO' );
      if ( typeof arg === 'string' && TypeIO.wrapForPhetioCommandProcessor ) {
        return this.wrapFunction( targetWindow, arg, TypeIO, request );
      }
      else {
        assert && assert( TypeIO.supportsDataTypeSerialization );
        return TypeIO.fromStateObject( arg );
      }
    } );

    // Invoke the method.  If it throws an error, it will be caught in handleSingleInvoke and returned to the client
    // See https://github.com/phetsims/phet-io/issues/941
    const result = methodSignature.implementation.apply( wrapper, args );

    // Serialize the result if possible.
    return methodSignature.returnType.toStateObject( result );
  }

[samreid]

I reverted usages of createIOType and deleted it. This should block publication until those changes have been reviewed. I'd like to request a review for the changes in Action.js and Emitter.js by @zepumph before NS SHAs. If you don't have time, let's ask @pixelzoom if he can review this. Once that is complete, this issue can be closed. Side tasks will be moved to new issues as described below.

Summarizing results from this issue:

• Automatically forwarding to a core type doesn't work well, because it is too tricky and is confusing regarding when to call IOType.superMethod vs CoreType.method. Manually written delegation methods are clear and succinct.
• We prefer to move the IO types to the bottom of the core type file. This makes it possible to set the validator using the exact class without needing to access it lazily or access it from the namespace. I'll make a new issue to move to this pattern.
• We need to investigate improved ways to describe IO types, how to describe an implement an interface. This will be pursued in Alternative ways to specify IO Types #211
• We can use setIOTypeFields for now, but ultimately may abandon that once we can declare static class attributes. The direction in Alternative ways to specify IO Types #211 will determine how we proceed on this aspect.
• The delegation methods are meant to be succinct, and do not warrant elaborate JSDoc. Previously we used to have:

class BunnyIO extends ObjectIO {

  /**
   * Serializes a Bunny instance.
   * @param {Bunny} bunny
   * @returns {Object}
   * @public
   * @override
   */
  static toStateObject( bunny ) {
    validate( bunny, this.validator );
    return bunny.toStateObject();
  }

  /**
   * Creates the args to BunnyGroup.createNextElement that creates Bunny instances.
   * @param state
   * @returns {Object[]}
   * @public
   * @override
   */
  static stateToArgsForConstructor( state ) {
    return Bunny.stateToArgsForConstructor( state );
  }

  /**
   * Restores Bunny state after instantiation.
   * @param {Bunny} bunny
   * @param {Object} state
   * @public
   * @override
   */
  static applyState( bunny, state ) {
    validate( bunny, this.validator );
    bunny.applyState( state );
  }
}

This has been compressed to:

class BunnyIO extends ObjectIO {

  // @public @override
  static toStateObject( bunny ) { return bunny.toStateObject(); }

  // @public @override
  static stateToArgsForConstructor( state ) { return Bunny.stateToArgsForConstructor( state ); }

  // @public @override
  static applyState( bunny, stateObject ) { bunny.applyState( stateObject ); }
}

[pixelzoom]

@samreid said:

Previously we used to have:
...
This has been compressed to:

There's not any real compression here. The compression is due to 3 questionable things in the example:

• omission of validate calls, likely may not be an improvement
• abbreviation of method doc and omission of @param annotation, again likely not an improvement
• defining what's left of the method on 1 line

So the code is essentially identical, and there's really no difference here.

Any compression was actually achieved in definition of static fields. For example this:

 BunnyIO.documentation = 'IO Type for Bunny';
 BunnyIO.validator = { isValidValue: value => value instanceof Bunny };
 BunnyIO.typeName = 'BunnyIO';
 ObjectIO.validateSubtype( BunnyIO );

was replaced with:

ObjectIO.setIOTypeFields( BunnyIO, 'BunnyIO', Bunny );

Otherwise the IO Types were just moved to the same .js file as their associated Core Type. And they could just as easily have been left in their own .js files. It's really only necessary to move the IO Type definition into the Core Type .js file if you need to avoid circular references between IO Type and Core Type. EDIT: But I think it improves maintainability by defining the Core Type and IO Type in the same .js file.

[samreid]

I've written a proposal in #211 (comment) which I think we should pursue. It automatically includes the validate calls, eliminates the need for JSDoc, and eliminates the need for setIOTypeFields and validateIOType. It's a larger change which will require changes to all IO Types and review, and we can hold off on it until Natural Selection has safe SHAs.

[zepumph]

Given the comment over in #211 (comment), this doesn't seem to block publication. @samreid, does this block NS?

[pixelzoom]

This issue is definitely not blocking for the 10/1 RC deliverable, the subject of phetsims/natural-selection#208. As soon as that RC has been delivered, then this issue will immediately becoming blocking for the next NS milestone (which is production RC).

I don't know about the blocking status for other sims.

[samreid]

Let's put this on hold pending work in #211.

[samreid]

Some brainstorming in #211 revolved around automatically leveraging IOType.valueType if it exists.

[samreid]

The latest proposal for this step is in #213, let's close this and continue there.