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

How to handle cases where PrivateStateUnderscored migration rule does nothing #341

Closed pixelzoom closed 1 year ago

pixelzoom commented 1 year ago

Related to migration rules in https://github.com/phetsims/natural-selection/issues/324 ...

This migration rule for BunnyIO was added to natural-selection-migration-rules.ts by @zepumph, and I added the TODO comment:

    //TODO https://github.com/phetsims/natural-selection/issues/324 will this trigger an assertion with ?strictMigrationRules if all bunnies have died?
    new PrivateStateUnderscored( 'BunnyIO' ),

Similarly, @zepumph added this rule and comment for WolfIO:

    // Note this will trigger an assertion with ?strictMigrationRules if there aren't any wolves migrating.
    new PrivateStateUnderscored( 'WolfIO' ),

For BunnyIO, I confirmed that running with ?strictMigrationRules will trigger an assertion failure if all bunnies have died. To reproduce:

  1. Run the sim in the Migration wrapper. Then in the old (top) version of the sim:
  2. Go to Intro screen.
  3. Check the "Wolves" checkbox
  4. Press the "Add a Mate" button
  5. Wait for wolves to kill all bunnies, and "All of the bunnies have died" dialog will be displayed.
  6. In the Migration wrapper, press the "Migrate Now" button. The following errors appear in the console:
Assertion failed:  MigrationRule did nothing in a setState call: 
    ConvertToStringsStateForLocale: Renames phetioIDs containing naturalSelection.homeScreen.view.titleText.textProperty -> naturalSelection.general.model.strings.naturalSelection.naturalSelection.titleStringProperty. The phetioID was renamed between versions, and the PhET-iO Element's state was forwarded to the new name for dynamic locale and string support in simulations. Most likely nothing else is needed for this migration rule, but if customizing in the previous version, it would be good to double check that the text is still customized in the new version.
window.assertions.assertFunction @ assert.js:24
ensureAllMigrationRulesDidSomething @ MigrationEngine.ts:197
(anonymous) @ MigrationEngine.ts:91
process @ MigrationEngine.ts:91
loadStandardPhetioWrapperFromHTML @ StandardPhetioWrapper.ts:315
await in loadStandardPhetioWrapperFromHTML (async)
loadStandardPhetioWrapperFromHTML @ Client.ts:1524
(anonymous) @ ?sim=natural-selection&locales=*&keyboardLocaleSwitcher&phetioDebug=true&phetioWrapperDebug=true&strictMigrationRules:241

assert.js:28 Uncaught (in promise) Error: Assertion failed: MigrationRule did nothing in a setState call: 
    ConvertToStringsStateForLocale: Renames phetioIDs containing naturalSelection.homeScreen.view.titleText.textProperty -> naturalSelection.general.model.strings.naturalSelection.naturalSelection.titleStringProperty. The phetioID was renamed between versions, and the PhET-iO Element's state was forwarded to the new name for dynamic locale and string support in simulations. Most likely nothing else is needed for this migration rule, but if customizing in the previous version, it would be good to double check that the text is still customized in the new version.
    at window.assertions.assertFunction (assert.js:28:13)
    at MigrationEngine.ensureAllMigrationRulesDidSomething (MigrationEngine.ts:197:19)
    at MigrationEngine.ts:91:56
    at Array.forEach (<anonymous>)
    at MigrationEngine.process (MigrationEngine.ts:91:28)
    at StandardPhetioWrapper.loadStandardPhetioWrapperFromHTML (StandardPhetioWrapper.ts:315:38)
    at async Client.loadStandardPhetioWrapperFromHTML (Client.ts:1524:20)
    at async ?sim=natural-selection&locales=*&keyboardLocaleSwitcher&phetioDebug=true&phetioWrapperDebug=true&strictMigrationRules:241:13
window.assertions.assertFunction @ assert.js:28
ensureAllMigrationRulesDidSomething @ MigrationEngine.ts:197
(anonymous) @ MigrationEngine.ts:91
process @ MigrationEngine.ts:91
loadStandardPhetioWrapperFromHTML @ StandardPhetioWrapper.ts:315

@zepumph please advise on how to address this. Is a comment similar to PrivateStateUnderscored( 'WolfIO' ) sufficient? Perhaps we need an option to tell migration that it's OK if a rule does nothing?

pixelzoom commented 1 year ago

Regarding the assertion failures shown in https://github.com/phetsims/natural-selection/issues/341#issue-1821171768 ... It looks like these are actually related to naturalSelection.titleStringProperty, and they occur if I just start the sim and press "Migrate Now". So I'll a separate issue for that.

pixelzoom commented 1 year ago

See also https://github.com/phetsims/natural-selection/issues/342#issuecomment-1655809081 for a general question about ?strictMigrationRules.

zepumph commented 1 year ago

This is an identical problem to https://github.com/phetsims/natural-selection/issues/342, and a solution was prototyped over there. Let's work on the problem over there.

pixelzoom commented 1 year ago

Looks good, confirm with in Migration wrapper with &strictMigrationRules. Closing, thanks!