IOType can supply default state methods by using valueType

[zepumph]

This is explained in #211 (comment), but deserves its own issue for investigation.

[samreid]

Here is the relevant part of that comment:

If we automatically leverage valueType in ObjectIO like so:

ObjectIO = new IOType( 'ObjectIO', {
  isValidValue: () => true,
  supertype: null,
  documentation: 'The root of the IO Type hierarchy',
  toStateObject: function( coreObject ) {
    return coreObject.toStateObject ? coreObject.toStateObject() : coreObject;
  },
  fromStateObject: function( stateObject ) {
    return this.valueType && this.valueType.fromStateObject ? this.valueType.fromStateObject( stateObject ) : stateObject;
  },
  stateToArgsForConstructor: function( stateObject ) {
    return this.valueType && this.valueType.stateToArgsForConstructor ? this.valueType.stateToArgsForConstructor( stateObject ) : [];
  },
  applyState: function( coreObject, stateObject ) {
    coreObject.applyState ? coreObject.applyState( stateObject ) : null;
  }
} );

Then 7 IO Type declarations in Natural Selection change from looking like this:

Wolf.WolfIO = new IOType( 'WolfIO', {
  valueType: Wolf,
  toStateObject: wolf => wolf.toStateObject(),
  stateToArgsForConstructor: Wolf.stateToArgsForConstructor,
  applyState: ( wolf, stateObject ) => wolf.applyState( stateObject )
} );

to looking like this:

Wolf.WolfIO = new IOType( 'WolfIO', {
  valueType: Wolf
} );

[samreid]

Why does it make sense to take some things from the core type, such as CoreType.fromStateObject, but other things cannot be taken from the core type, such as CoreType.phetioMethods being taken as the IO Type methods or CoreType.phetioDocumentation being taken as the IO Type documentation?

[samreid]

Note the philosophy for this issue was introduced in #188, may be good to skim that issue before closing this one, to make sure our bases are covered.

[samreid]

@zepumph and I discussed these APIs

// Reasonable
Bunny.BunnyIO = new IOType( 'BunnyIO', {
  valueType: Bunny,
  fromCoreType: true
} );

// Possibly better?
Bunny.BunnyIO = IOType.fromCoreType( 'BunnyIO', Bunny );

[samreid]

It may be more seamless to move static methods to the core types if we support static attribute, see phetsims/tasks#1048

[samreid]

@pixelzoom can you please review the proposal in #213 (comment) ? Does it seem too tricky/magical to you? Or an efficient way to eliminate unnecessary code?

[pixelzoom]

Can you explain what an IO Type does other than serialization and type verification? If the IO Type is delegating to the Core Type, and the IO Type doesn't have other responsibilities, then why is an IO Type necessary?

[samreid]

In addition to serialization, IO types form a type system for PhET-iO Elements for wrapper interoperability. They describe methods, documentation, type hierarchy & events that can be emitted.

[pixelzoom]

Could those responsibilities also be folded into the Core Type, or is there value in keeping them separate?

[samreid]

There are parts that make that difficult or impossible, such as:

• IO Types that don't correspond to a PhetioObject, such as NumberIO, StringIO, VoidIO, ArrayIO, BooleanIO, FunctionIO, Float64ArrayIO.
• Parametric IO Types, such as PropertyIO<Vector2IO>
• Nullable IO Types, such as NullableIO<StringIO>

That being said, in some cases we could move some things to static elements on the core type. Since we don't support static attributes yet, the syntax could look like:

CoreType.phetioDocumentation = 'An element that etc.etc.';
new IOType('CoreTypeIO',{
   valueType: CoreType
})

as opposed to the current syntax:

new IOType('CoreTypeIO',{
   documentation: 'An element that etc.etc.
   valueType: CoreType
})

[pixelzoom]

OK, thanks for clarifying. So it sounds like doing away with IO Types altogether is not an option.

So back to your original question:

... Does it seem too tricky/magical to you? Or an efficient way to eliminate unnecessary code?

If I understand the various options that have been proposed... I like having both the "non-magical" long form, with a factory method for the "magical" short form. For example:

// Long-form
Wolf.WolfIO = new IOType( 'WolfIO', {
  valueType: Wolf,
  toStateObject: wolf => wolf.toStateObject(),
  stateToArgsForConstructor: Wolf.stateToArgsForConstructor,
  applyState: ( wolf, stateObject ) => wolf.applyState( stateObject )
} );

// Short-form, convenience
Wolf.WolfIO = IOType.fromCoreType( 'WolfIO', Wolf, { ... } );

[zepumph]

I really like this idea! How about the above commit. Please review. I'm mainly wondering if it is too strict to make sure that options doesn't have all 4 of these things:

    if ( assert && options ) {
      assert && assert( !options.hasOwnProperty( 'valueType' ), 'fromCoreType sets its own valueType' );
      assert && assert( !options.hasOwnProperty( 'toStateObject' ), 'fromCoreType sets its own toStateObject' );
      assert && assert( !options.hasOwnProperty( 'stateToArgsForConstructor' ), 'fromCoreType sets its own stateToArgsForConstructor' );
      assert && assert( !options.hasOwnProperty( 'applyState' ), 'fromCoreType sets its own applyState' );
    }

[pixelzoom]

@zepumph asked:

... I'm mainly wondering if it is too strict to make sure that options doesn't have all 4 of these things:

I think that's perfect for now. If we find a reason that one of those options should be applied, we can change later.

Question... I see this in fromCoreType:

225 stateToArgsForConstructor: CoreType.stateToArgsForConstructor,

What happens in CoreType doesn't have stateToArgsForConstructor? I'd like to fromCoreType for several other IO Types in Natural Selection that have this boilerplate, and the Core Type does not define stateToArgsForConstructor:

GenePair.GenePairIO = new IOType( 'GenePairIO', {
  valueType: GenePair,
  toStateObject: genePair => genePair.toStateObject(),
  applyState: ( genePair, stateObject ) => genePair.applyState( stateObject )
} );

[zepumph]

What about this patch?

  static fromCoreType( ioTypeName, CoreType, options ) {

    if ( assert && options ) {
      assert && assert( !options.hasOwnProperty( 'valueType' ), 'fromCoreType sets its own valueType' );
      assert && assert( !options.hasOwnProperty( 'toStateObject' ), 'fromCoreType sets its own toStateObject' );
      assert && assert( !options.hasOwnProperty( 'stateToArgsForConstructor' ), 'fromCoreType sets its own stateToArgsForConstructor' );
      assert && assert( !options.hasOwnProperty( 'applyState' ), 'fromCoreType sets its own applyState' );
    }

    options = merge( {
      valueType: CoreType,
      toStateObject: coreType => coreType.toStateObject(),
      forwardStateToArgesForConstructor: true, // Only needed for dynamic-element deserialization
      forwardApplyState: true // Only needed for reference-type deserialization
    }, options );

    if ( options.forwardApplyState ) {
      options.applyState = ( coreType, stateObject ) => coreType.applyState( stateObject );
    }
    if ( options.forwardStateToArgesForConstructor ) {
      options.stateToArgsForConstructor = CoreType.stateToArgsForConstructor;
    }
    return new IOType( ioTypeName, options );
  }

?

[pixelzoom]

What about this patch?

I don't like forwardStateToArgesForConstructor and forwardApplyState, they are a little too "indirect" for my tastes.

I also don't understand this line in the patch:

forwardApplyState: true // Only needed for reference-type deserialization

Is that comment correct? WolfIO uses dynamic element serialization, and it defines options.applyState. And is applyState really optional with fromCoreType? I guess I'm not clear on which serialization situations use fromCoreType, and that should be documented.

With those caveats, and if stateToArgsForConstructor and applyState are truly optional....

How about this implementation?

  static fromCoreType( ioTypeName, CoreType, options ) {

    if ( assert && options ) {
      assert && assert( !options.hasOwnProperty( 'valueType' ), 'fromCoreType sets its own valueType' );
      assert && assert( !options.hasOwnProperty( 'toStateObject' ), 'fromCoreType sets its own toStateObject' );
      assert && assert( !options.hasOwnProperty( 'stateToArgsForConstructor' ), 'fromCoreType sets its own stateToArgsForConstructor' );
      assert && assert( !options.hasOwnProperty( 'applyState' ), 'fromCoreType sets its own applyState' );
    }

    assert && assert( CoreType.prototype.toStateObject, 'class CoreType must implement method toStateObject' );

    options = merge( {
      valueType: CoreType,
      toStateObject: coreType => coreType.toStateObject(),
    }, options );

    if ( CoreType.stateToArgsForConstructor ) {
      options.stateToArgsForConstructor = CoreType.stateToArgsForConstructor;
    }

    if ( CoreType.prototype.applyState ) {
      options.applyState = ( coreType, stateObject ) => coreType.applyState( stateObject );
    }

    return new IOType( ioTypeName, options );
  }
}

One more nit... options should be config in all of the above, because the param to IOType constructor is config.

[zepumph]

Sounds reasonable to me!

One small issue:

    if ( CoreType.prototype.applyState ) {
      options.applyState = ( coreType, stateObject ) => coreType.applyState( stateObject );
    }

This will not look up the hierarchy. We could recurse though.

[pixelzoom]

Good point. And this will also not recurse (in case you missed this addition):

    assert && assert( CoreType.prototype.toStateObject, 'class CoreType must implement method toStateObject' );

[zepumph]

We can use Inheritance.js as an example of how to recurse for these checks.

[pixelzoom]

@zepumph if you need test cases for fromCoreType, feel free to modify the Natural Selection IO Types listed in phetsims/natural-selection#255.

[zepumph]

One more nit... options should be config in all of the above, because the param to IOType constructor is config.

I disagree. fromCoreType doesn't require anything to be provided via its options, because it sets the require pieces in fromCoreType (valueType).

I added documentation saying that toStateObject is required, but any deserialization method is gracefully supported. I felt good enough to convert all usages worth it in Natural Selection, and I also converted Vector2 to test fromStateObject (which I added support for too).

@pixelzoom please review.

[pixelzoom]

One more nit... options should be config in all of the above, because the param to IOType constructor is config.

I disagree. fromCoreType doesn't require anything to be provided via its options, because it sets the require pieces in fromCoreType (valueType).

OK. I took it for granted that the config param to fromCoreType was correct, but that's apparently not the case since you changed it to options. In any case, they params are now consistent, and I don't see anything config-y in fromCoreType, so bravo!

Closing.