phetsims / natural-selection

"Natural Selection" is an educational simulation in HTML5, by PhET Interactive Simulations
GNU General Public License v3.0
3 stars 7 forks source link

address boilerplate in creating IO Types #218

Closed pixelzoom closed 4 years ago

pixelzoom commented 4 years ago

In https://github.com/phetsims/tandem/issues/188#issuecomment-685914398, @zepumph said:

This pattern is looking really nice! @samreid took me through it today. We think it is ready to be applied to all IO types. I think that it makes the most sense to name this the same for each type, like IOType, and then for parameteric types something like createIOType(). ...

On hold until that issue is green-lighted.

This is not required for 10/1 deadline publication, but highly desirable.

samreid commented 4 years ago

I moved the parentIOType to the options and lowercased it. This issue is no longer on hold.

pixelzoom commented 4 years ago

Problems:

(1) I don't seem to be able to replace subclasses of ReferenceIO, see https://github.com/phetsims/tandem/issues/188#issuecomment-686246212. Does createIOType not work with ReferenceIO or parameterized IO Types?

(2) All of my Core Types are missing one or more required methods, and fail way down the line in State Wrapper, see https://github.com/phetsims/tandem/issues/188#issuecomment-686256266.

(3) I noticed that BunnyCounts does not extend PhetioObject, and seemed to have been working OK all this time. Should it extend PhetioObject? If not, I should document why.

I did not committed this work, and reverted my working copy.

pixelzoom commented 4 years ago

This issue is on hold again until phetsims/tandem#188 issues are addressed.

pixelzoom commented 4 years ago

I bailed on using ObjectIO.createIOType and wrote my own createIOType.js that now creates all of Natural Selection's IO Types, including parameterized ReferenceIO subclasses.

I'll note in phetsims/tandem#188, and the iO team is welcome to borrow from this for ObjectIO.createIOType. If ObjectIO.createIOType is cleaned up and addresses all of the needs of this Natural Selection, I'll be happy to revisit using it. Until then, closing this issue.

pixelzoom commented 4 years ago

A few example uses of createIOType.js:

BunnyCounts.BunnyCountsIO = createIOType( BunnyCounts, 'BunnyCountsIO', {
  toStateObject: bunnyCounts => bunnyCounts.toStateObject(),
  fromStateObject: stateObject => BunnyCounts.fromStateObject( stateObject )
} );
Genotype.GenotypeIO = createIOType( Genotype, 'GenotypeIO', {
  toStateObject: genotype => genotype.toStateObject(),
  applyState: ( genotype, stateObject ) => genotype.applyState( stateObject )
} );
Allele.AlleleIO = createIOType( Allele, 'AlleleIO', {
  parentIOType: ReferenceIO( ObjectIO )
} );
pixelzoom commented 4 years ago

Reopening to address this TODO:

    //TODO https://github.com/phetsims/natural-selection/issues/218 doc, not currently used in natural-selection
    methods: {},
    events: [],
    parameterTypes: []
pixelzoom commented 4 years ago

Addressed, closing.

pixelzoom commented 4 years ago

Reopening to explore a different approach that I outlined in https://github.com/phetsims/tandem/issues/188#issuecomment-687163984.

pixelzoom commented 4 years ago

Now using an approach throughout that looks like this. class extends is used to define the class, setIOTypeFields is used to set (boilerplate) static constants in the IO Type.

/**
 * BunnyIO handles PhET-iO serialization of Bunny. Because serialization involves accessing private members,
 * it delegates to Bunny. The methods that BunnyIO overrides are typical of 'Dynamic element serialization',
 * as described in the Serialization section of
 * https://github.com/phetsims/phet-io/blob/master/doc/phet-io-instrumentation-guide.md#serialization
 */
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 ); }
}

setIOTypeFields( BunnyIO, 'BunnyIO', Bunny );

// @public
Bunny.BunnyIO = BunnyIO;
pixelzoom commented 4 years ago

On hold pending further work on phetsims/tandem#188. This doesn't block 10/1 dev version deliverable.

pixelzoom commented 4 years ago

This seems to be in a good place now, so I'm going to close this issue.

IO Types in general are likely to evolve as work on phetsims/tandem#188 continues. But we'll open a new issue if it requires NS changes.