phetsims / tandem

Simulation-side code for PhET-iO
MIT License
0 stars 5 forks source link

defaultApplyState doesn't support setting private variables that start with `_`. #297

Closed zepumph closed 1 year ago

zepumph commented 1 year ago

Over in https://github.com/phetsims/natural-selection/issues/327, We want to make a few Bunny fields private, with a public getter, so a private settable _age and the a public get age. It would be awesome if the defaultApplyState supported setting on the underscored key.

samreid commented 1 year ago

I was wondering if we would allow a more general remapping, like '_myValuemaps toMyClass.someValue`? Also with the proposal above, how will it autodetect or how will we specify which ones have a different mapping?

zepumph commented 1 year ago

That may be nice, but in general, and mostly for maintainability, it has been really nice to have the stateObject keys match the class fields. With or without an underscore doesn't effect that for me too much. This parallels our convention of having tandem names match variable names.

In this patch we just see if the core object has the underscore setter.

```diff Subject: [PATCH] respect first letter casing of the string key, https://github.com/phetsims/chipper/issues/1394 --- Index: js/types/StateSchema.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/types/StateSchema.ts b/js/types/StateSchema.ts --- a/js/types/StateSchema.ts (revision d9a3e31160def56aeda540224e5912f8ac2cb66d) +++ b/js/types/StateSchema.ts (date 1685988243710) @@ -65,13 +65,29 @@ assert && assert( this.isComposite(), 'defaultApplyState from stateSchema only applies to composite stateSchemas' ); for ( const stateKey in this.compositeSchema ) { if ( this.compositeSchema.hasOwnProperty( stateKey ) ) { - assert && assert( stateObject.hasOwnProperty( stateKey ), `stateObject does not have expected schema key: ${stateKey}` ); + + // Does the class field start with an underscore? We need to cover two cases here. The first is where the underscore + // was added to make a private state key. The second, is where the core class only has the underscore-prefixed + // field key name available for setting. The easiest algorithm to cover all cases is to see if the coreObject has + // the underscore-prefixed key name, and use that if available, otherwise use the stateKey without an underscore. + const noUnderscore = stateKey.startsWith( '_' ) ? stateKey.substring( 1 ) : stateKey; + const underscored = `_${stateKey}`; + let coreObjectAccessor: string; + + // TODO: support es5 setters too? Not sure we would ever have a `public set _fieldName()` https://github.com/phetsims/tandem/issues/297 + // @ts-expect-error - T is not specific to composite schemas, so NumberIO doesn't actually need a hasOwnProperty method + if ( coreObject.hasOwnProperty( underscored ) ) { + coreObjectAccessor = underscored; + } + else { + coreObjectAccessor = noUnderscore; + } + + assert && assert( stateObject.hasOwnProperty( coreObjectAccessor ), `stateObject does not have expected schema key: ${stateKey}` ); // The IOType for the key in the composite. const schemaIOType = this.compositeSchema[ stateKey ]; - const coreObjectAccessor = stateKey.startsWith( '_' ) ? stateKey.substring( 1 ) : stateKey; - // Using fromStateObject to deserialize sub-component if ( schemaIOType.defaultDeserializationMethod === 'fromStateObject' ) {
zepumph commented 1 year ago

Discussed with @samreid and it seems alright and pretty simple to implement.

zepumph commented 1 year ago
```diff Subject: [PATCH] implement a min deflection for photons passing through layers, see https://github.com/phetsims/greenhouse-effect/issues/167 --- Index: js/common/model/Photon.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/common/model/Photon.ts b/js/common/model/Photon.ts --- a/js/common/model/Photon.ts (revision 4768de9c083f15ee010af67e1dc4e875c0b356c2) +++ b/js/common/model/Photon.ts (date 1687371538923) @@ -123,16 +123,12 @@ this.previousPosition.set( this.positionProperty.value ); } - // Provide a custom toStateObject method because the default doesn't support mapping a composite position property - // into our state object. - public toStateObject(): PhotonStateObject { - return { - position: this.positionProperty.value.toStateObject(), - previousPosition: this.previousPosition.toStateObject(), - wavelength: this.wavelength, - velocity: this.velocity.toStateObject(), - showState: EnumerationIO( ShowState ).toStateObject( this.showState ) - }; + // Setters and getters needed for phet-io. + public set position( position: Vector2 ) { + this.positionProperty.set( position ); + } + public get position(): Vector2 { + return this.positionProperty.get(); } // static values @@ -141,7 +137,9 @@ public static readonly SPEED = PHOTON_SPEED; public static readonly ShowState = ShowState; - // phet-io + // IOType for data-type serialization. This type of serialization is used because we don't need to provide + // information on photons, or the ability to directly manipulate them individually, to phet-io users. This also has + // higher performance versus having every photon individually instrumented as a phet-io object. public static readonly PhotonIO = new IOType( 'PhotonIO', { valueType: Photon, stateSchema: { @@ -151,7 +149,6 @@ velocity: Vector2.Vector2IO, showState: StringIO }, - toStateObject: ( photon: Photon ) => photon.toStateObject(), fromStateObject: ( stateObject: PhotonStateObject ) => new Photon( Vector2.fromStateObject( stateObject.position ), stateObject.wavelength, { ```
zepumph commented 1 year ago

@samreid can you please review. I think it is best and easiest to get a handle of this issue in the unit test. I'm trying to be as complete as possible, but we do not support an es5 getter for an underscore-prefixed name. I feel like that is totally fine myself.

samreid commented 1 year ago

@zepumph and I reviewed and made some improvements. Anything else for this issue?