search

phetsims / gravity-and-orbits

"Gravity And Orbits" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
12 stars 6 forks source link

Fix PhET-iO migration #483

Closed zepumph closed 1 year ago

zepumph commented 1 year ago

Tagging https://github.com/phetsims/phet-io/issues/1960

zepumph commented 1 year ago

When migrating from 1.5-> 1.7

Assertion failed:  migration rules must handle all elements that converted to be LinkedElementIO.
gravityAndOrbits.modelScreen.nameProperty
gravityAndOrbits.toScaleScreen.nameProperty
gravityAndOrbits.toScaleScreen.view.playAreaNode.starPlanetSceneView.measuringTapeNode.visibleProperty
gravityAndOrbits.toScaleScreen.view.playAreaNode.starPlanetMoonSceneView.measuringTapeNode.visibleProperty
gravityAndOrbits.toScaleScreen.view.playAreaNode.planetMoonSceneView.measuringTapeNode.visibleProperty
gravityAndOrbits.toScaleScreen.view.playAreaNode.planetSatelliteSceneView.measuringTapeNode.visibleProperty
zepumph commented 1 year ago

We got to a pretty good spot here, making some progress. We ran out of time with the fact that

gravityAndOrbits.modelScreen.model.starPlanetScene.clock.timeProperty

changed from NumberPropertyIO -> RewindablePropertyIO, and so no longer has a numberType attribute.

Assertion failed:  stateObject provided a key that is not in the schema: numberType

@samreid and I would like to use this opportunity to see if we can improve how new migration rule classes are created. We will touch base tomorrow.

samreid commented 1 year ago

Here is a working patch that fixes the Gravity and Orbits 1.5 -> 1.7 problem for the NumberProperty => RewindableProperty.

```diff Subject: [PATCH] Add migration rules, see https://github.com/phetsims/phet-io/issues/1960 --- Index: phet-io-sim-specific/repos/molecule-polarity/js/molecule-polarity-migration-rules.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/phet-io-sim-specific/repos/molecule-polarity/js/molecule-polarity-migration-rules.ts b/phet-io-sim-specific/repos/molecule-polarity/js/molecule-polarity-migration-rules.ts --- a/phet-io-sim-specific/repos/molecule-polarity/js/molecule-polarity-migration-rules.ts (revision a1182b5a3d0071664406f1571536e3c80f57c330) +++ b/phet-io-sim-specific/repos/molecule-polarity/js/molecule-polarity-migration-rules.ts (date 1693964876534) @@ -23,7 +23,7 @@ // NumberProperty had a step attribute to tell Studio how much to increment/decrement sliders, but it was removed. // See https://github.com/phetsims/axon/issues/386 - new IOTypeStateAttributeDelete( 'NumberPropertyIO', 'step' ), + new IOTypeStateAttributeDelete( 'NumberPropertyIO', null, 'step' ), // The default range for NumberProperty used to be range: null, but is now range: [-Infinity,+Infinity]. // See https://github.com/phetsims/axon/issues/425 Index: phet-io-sim-specific/repos/ph-scale-basics/js/ph-scale-basics-migration-rules.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/phet-io-sim-specific/repos/ph-scale-basics/js/ph-scale-basics-migration-rules.ts b/phet-io-sim-specific/repos/ph-scale-basics/js/ph-scale-basics-migration-rules.ts --- a/phet-io-sim-specific/repos/ph-scale-basics/js/ph-scale-basics-migration-rules.ts (revision a1182b5a3d0071664406f1571536e3c80f57c330) +++ b/phet-io-sim-specific/repos/ph-scale-basics/js/ph-scale-basics-migration-rules.ts (date 1693964876550) @@ -25,10 +25,10 @@ // NumberProperty used to have a step attribute to tell studio how much to increment/decrement sliders, but // it was removed in https://github.com/phetsims/axon/issues/386 - new IOTypeStateAttributeDelete( 'NumberPropertyIO', 'step' ), + new IOTypeStateAttributeDelete( 'NumberPropertyIO', null, 'step' ), // SoluteIO used to have a name attribute, but it was removed in https://github.com/phetsims/ph-scale/issues/243 - new IOTypeStateAttributeDelete( 'SoluteIO', 'name' ), + new IOTypeStateAttributeDelete( 'SoluteIO', null, 'name' ), new ConvertToStringsStateForLocale( 'phScaleBasics.general.view.navigationBar.titleText.textProperty', 'phScaleBasics.general.model.strings.phScaleBasics.phScaleBasics.titleStringProperty', stringsStatePhetioID ), Index: phet-io-sim-specific/repos/gravity-and-orbits/js/gravity-and-orbits-migration-rules.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/phet-io-sim-specific/repos/gravity-and-orbits/js/gravity-and-orbits-migration-rules.ts b/phet-io-sim-specific/repos/gravity-and-orbits/js/gravity-and-orbits-migration-rules.ts --- a/phet-io-sim-specific/repos/gravity-and-orbits/js/gravity-and-orbits-migration-rules.ts (revision a1182b5a3d0071664406f1571536e3c80f57c330) +++ b/phet-io-sim-specific/repos/gravity-and-orbits/js/gravity-and-orbits-migration-rules.ts (date 1693965040182) @@ -156,7 +156,10 @@ // NumberProperty had a step attribute to tell Studio how much to increment/decrement sliders, but it was removed. // See https://github.com/phetsims/axon/issues/386 - new IOTypeStateAttributeDelete( 'NumberPropertyIO', 'step' ), + new IOTypeStateAttributeDelete( 'NumberPropertyIO', null, 'step' ), + new IOTypeStateAttributeDelete( 'NumberPropertyIO', 'RewindablePropertyIO', 'numberType' ), + new IOTypeStateAttributeDelete( 'NumberPropertyIO', 'RewindablePropertyIO', 'range' ), + new IOTypeStateAttributeDelete( 'NumberPropertyIO', 'RewindablePropertyIO', 'rangePhetioID' ), new PhetioTypeDelete( 'SimInfoIO' ) // must be last after all renames so that the phetioID matches in the newAPI ], Index: phet-io-sim-specific/repos/ph-scale/js/ph-scale-migration-rules.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/phet-io-sim-specific/repos/ph-scale/js/ph-scale-migration-rules.ts b/phet-io-sim-specific/repos/ph-scale/js/ph-scale-migration-rules.ts --- a/phet-io-sim-specific/repos/ph-scale/js/ph-scale-migration-rules.ts (revision a1182b5a3d0071664406f1571536e3c80f57c330) +++ b/phet-io-sim-specific/repos/ph-scale/js/ph-scale-migration-rules.ts (date 1693964876528) @@ -24,10 +24,10 @@ // NumberProperty used to have a step attribute to tell studio how much to increment/decrement sliders, but // it was removed in https://github.com/phetsims/axon/issues/386 - new IOTypeStateAttributeDelete( 'NumberPropertyIO', 'step' ), + new IOTypeStateAttributeDelete( 'NumberPropertyIO', null, 'step' ), // SoluteIO used to have a name attribute, but it was removed in https://github.com/phetsims/ph-scale/issues/243 - new IOTypeStateAttributeDelete( 'SoluteIO', 'name' ), + new IOTypeStateAttributeDelete( 'SoluteIO', null, 'name' ), // Screen renames new ConvertToStringsStateForLocale( /phScale\.general\.view\.navigationBar\.(.*)ScreenButton\.text\.textProperty/, Index: phet-io-wrappers/common/js/migration/IOTypeStateAttributeDelete.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/phet-io-wrappers/common/js/migration/IOTypeStateAttributeDelete.ts b/phet-io-wrappers/common/js/migration/IOTypeStateAttributeDelete.ts --- a/phet-io-wrappers/common/js/migration/IOTypeStateAttributeDelete.ts (revision ab11026016485fd02e50c03c0b4e1adc8e2bf1b3) +++ b/phet-io-wrappers/common/js/migration/IOTypeStateAttributeDelete.ts (date 1693965006232) @@ -16,16 +16,16 @@ import MigrationEngine from './MigrationEngine.js'; export default class IOTypeStateAttributeDelete extends MigrationRule { - public constructor( public readonly ioTypeName: string, public readonly key: string, options?: MigrationRuleOptions ) { + public constructor( public readonly ioTypeName: string, public readonly newIOTypeName: string | null, public readonly key: string, options?: MigrationRuleOptions ) { super( options ); } public toString(): string { - return `IOTypeStateAttributeDelete[${this.ioTypeName},${this.key}]: removes this value from Property state for validation. No further action required.`; + return `IOTypeStateAttributeDelete[${this.ioTypeName},${this.newIOTypeName},${this.key}]: removes this value from Property state for validation. No further action required.`; } public isEqual( m: MigrationRule ): boolean { - return m instanceof IOTypeStateAttributeDelete && this.ioTypeName === m.ioTypeName && this.key === m.key; + return m instanceof IOTypeStateAttributeDelete && this.ioTypeName === m.ioTypeName && this.key === m.key && this.newIOTypeName === m.newIOTypeName; } private nameAppearsAnywhere( ioTypeName: string ): boolean { @@ -58,27 +58,31 @@ // This guard is needed because the "oldState" is likely mutated by MigrationRules that came before this one, // and could have "new" state keys in it that aren't in the oldAPI. if ( oldFlatAPI.hasOwnProperty( archetypalPhetioID ) ) { - const ioTypeName = oldFlatAPI[ archetypalPhetioID ]._metadata.phetioTypeName; + const oldIOTypeName = oldFlatAPI[ archetypalPhetioID ]._metadata.phetioTypeName; + const newIOTypeName = newFlatAPI[ archetypalPhetioID ]?._metadata?.phetioTypeName; - if ( this.nameAppearsAnywhere( ioTypeName ) ) { - if ( this.matchesTopLevelTypeName( ioTypeName ) ) { + if ( this.newIOTypeName === null || ( newIOTypeName && newIOTypeName.includes( this.newIOTypeName ) ) ) { + + if ( this.nameAppearsAnywhere( oldIOTypeName ) ) { + if ( this.matchesTopLevelTypeName( oldIOTypeName ) ) { - assert && assert( oldState.hasOwnProperty( keyPhetioID ) && newState[ keyPhetioID ].hasOwnProperty( this.key ), `Expected state for ${keyPhetioID} in oldState` ); + assert && assert( oldState.hasOwnProperty( keyPhetioID ) && newState[ keyPhetioID ].hasOwnProperty( this.key ), `Expected state for ${keyPhetioID} in oldState` ); - delete newState[ keyPhetioID ][ this.key ]; - messages.push( `IOTypeStateAttributeDelete[${this.ioTypeName},${this.key}]: ` + keyPhetioID ); - } - else if ( this.matchesPropertyValueTypeName( ioTypeName ) ) { + delete newState[ keyPhetioID ][ this.key ]; + messages.push( `IOTypeStateAttributeDelete[${this.ioTypeName},${this.key}]: ` + keyPhetioID ); + } + else if ( this.matchesPropertyValueTypeName( oldIOTypeName ) ) { - // We need to also check for the nested .value attribute if it is a PropertyIO - const presentInNestedValue = oldState.hasOwnProperty( keyPhetioID ) && oldState[ keyPhetioID ].value.hasOwnProperty( this.key ); - assert && assert( presentInNestedValue, `Expected state for ${keyPhetioID} in oldState` ); + // We need to also check for the nested .value attribute if it is a PropertyIO + const presentInNestedValue = oldState.hasOwnProperty( keyPhetioID ) && oldState[ keyPhetioID ].value.hasOwnProperty( this.key ); + assert && assert( presentInNestedValue, `Expected state for ${keyPhetioID} in oldState` ); - delete newState[ keyPhetioID ].value[ this.key ]; - messages.push( `IOTypeStateAttributeDelete[${this.ioTypeName},${this.key}]: ` + keyPhetioID ); - } - else { - assert && assert( false, `Name appeared, but there was no handler: ${ioTypeName} to delete state attributes` ); + delete newState[ keyPhetioID ].value[ this.key ]; + messages.push( `IOTypeStateAttributeDelete[${this.ioTypeName},${this.key}]: ` + keyPhetioID ); + } + else { + assert && assert( false, `Name appeared, but there was no handler: ${oldIOTypeName} to delete state attributes` ); + } } } } Index: phet-io-sim-specific/repos/natural-selection/js/natural-selection-migration-rules.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/phet-io-sim-specific/repos/natural-selection/js/natural-selection-migration-rules.ts b/phet-io-sim-specific/repos/natural-selection/js/natural-selection-migration-rules.ts --- a/phet-io-sim-specific/repos/natural-selection/js/natural-selection-migration-rules.ts (revision a1182b5a3d0071664406f1571536e3c80f57c330) +++ b/phet-io-sim-specific/repos/natural-selection/js/natural-selection-migration-rules.ts (date 1693964876547) @@ -25,7 +25,7 @@ // NumberProperty had a step attribute to tell Studio how much to increment/decrement sliders, but it was removed. // See https://github.com/phetsims/axon/issues/386 - new IOTypeStateAttributeDelete( 'NumberPropertyIO', 'step' ), + new IOTypeStateAttributeDelete( 'NumberPropertyIO', null, 'step' ), // The default range for NumberProperty used to be range: null, but is now range: [-Infinity,+Infinity]. // See https://github.com/phetsims/axon/issues/425 Index: phet-io-sim-specific/repos/graphing-quadratics/js/graphing-quadratics-migration-rules.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/phet-io-sim-specific/repos/graphing-quadratics/js/graphing-quadratics-migration-rules.ts b/phet-io-sim-specific/repos/graphing-quadratics/js/graphing-quadratics-migration-rules.ts --- a/phet-io-sim-specific/repos/graphing-quadratics/js/graphing-quadratics-migration-rules.ts (revision a1182b5a3d0071664406f1571536e3c80f57c330) +++ b/phet-io-sim-specific/repos/graphing-quadratics/js/graphing-quadratics-migration-rules.ts (date 1693964876538) @@ -21,7 +21,7 @@ // NumberProperty had a step attribute to tell Studio how much to increment/decrement sliders, but it was removed. // See https://github.com/phetsims/axon/issues/386 - new IOTypeStateAttributeDelete( 'NumberPropertyIO', 'step' ), + new IOTypeStateAttributeDelete( 'NumberPropertyIO', null,'step' ), // For screen names, nameProperty was replaced with a link to a StringProperty. // See https://github.com/phetsims/joist/issues/844 ```

I tried to keep the following objectives:

There are other ways to do this, we should discuss.

zepumph commented 1 year ago

Lots of good progress today expanding on @samreid's patch above. We created IOTypeChange, with hard coded mappings inside for how to handle each possible change. In the future we may want to factor these out, but the boilerplate to do so at this time didn't seem worth it.

Then! We were able to make a migration rule checker that asserts out whenever it finds a phetioID that has had a type change without being "handled" by a migration rule. This helped find multiple things that were sliding through the cracks before, though none were causing any trouble except for one in gravity and orbits (hence why it was failing on CT because it because a RewindableProperty).

Still to do and discuss with @samreid:

samreid commented 1 year ago

I'll move the main work of IOTypeChange to a method documented that we need to be careful not to change existing or legacy functionality.

samreid commented 1 year ago

From today's discussion:

```diff Subject: [PATCH] Factor out shared rules, see https://github.com/phetsims/circuit-construction-kit-common/issues/994 --- Index: common/js/migration/MigrationEngine.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/common/js/migration/MigrationEngine.ts b/common/js/migration/MigrationEngine.ts --- a/common/js/migration/MigrationEngine.ts (revision c13f023e02d44d26b65a206f378189937998c3a9) +++ b/common/js/migration/MigrationEngine.ts (date 1694106074794) @@ -83,6 +83,7 @@ const startTime = Date.now(); let command = new Command( input.phetioID, input.method, input.args ); + const oldCommand = new Command( input.phetioID, input.method, input.args ); const messageBatches: string[][] = []; @@ -92,7 +93,7 @@ messageBatches.push( result.messages ); } ); - this.migrationCheckers.forEach( checker => checker.call( this, command, messageBatches ) ); + this.migrationCheckers.forEach( checker => checker.call( this, oldCommand, command, messageBatches ) ); const endTime = Date.now(); const elapsed = endTime - startTime; @@ -186,8 +187,12 @@ // @ts-expect-error - it is a state in this case const state: object = command.args[ 0 ]; + // TODO: Iterate over the old state, not the new state Object.keys( state ).forEach( phetioID => { + // TODO: Promote the old phetioIDs so you know where to look them up in the new API. + // TODO: Do this by iterating over all the TandemFragmentRenamed rules (ignore ConvertToStringStateForLocale to begin) + // TODO: and rule.promote() the old phetioID to the new phetioID. const oldIOTypeName = this.oldAPIFlat[ phetioID ]?._metadata?.phetioTypeName; const newIOTypeName = this.newAPIFlat[ phetioID ]?._metadata?.phetioTypeName; ```
zepumph commented 1 year ago

All is done except: