search

phetsims / sun

User-interface components for PhET simulations, built on top of Scenery.
MIT License
4 stars 12 forks source link

TextPushButton change requires migration rules #854

Closed pixelzoom closed 11 months ago

pixelzoom commented 11 months ago

In https://github.com/phetsims/joist/issues/934#issuecomment-1688146369, @samreid reported that calculus-graph is failing Migration wrapper with:

assert.js:24 Assertion failed: These phetioIDs are not found in the new API. Either they were renamed incorrectly or were not migrated at all: calculusGrapher.advancedScreen.view.controlPanel.pushButtonGroup.smoothButton.text.stringProperty calculusGrapher.labScreen.view.controlPanel.pushButtonGroup.smoothButton.text.stringProperty

I investigated and discovered that this is due to https://github.com/phetsims/sun/commit/8484e9e3b1e3a304a67671232fbb7d15bd7ddade by @jbphet, where the Text node in TextPushButton.js was uninstrumented. This change should have been accompanied by migration rules in affected sims (one of which is calculus-grapher).

Since we decided that the developer making such a change is responsible for the migration rules, I'm assigning this to @jbphet. Top priority please. @samreid @zepumph FYI.

pixelzoom commented 11 months ago

In https://github.com/phetsims/joist/issues/934#issuecomment-1688177340, @samreid addressed this issue for calculus grapher. But it's likely that it still needs to be addressed in other PhET-iO sims. Since it's too late to use grunt generate-phet-io-api to identify affected sims, you may need to run the Migration wrapper for each PhET-iO sim to find out where a migration rule is needed.

samreid commented 11 months ago

We have not trained the team on how to develop migration rules, so should this issue be on hold until that training? Also it is my understanding is that the "priority:1-top" label designates that it is important enough to interrupt other scheduled work. Is that the intention here?

pixelzoom commented 11 months ago

We have not trained the team on how to develop migration rules, so should this issue be on hold until that training?

Based on https://github.com/phetsims/phet-io/issues/1955#issuecomment-1672095895, it looks to me like @jbphet is partially up to speed. So this issue seems like an opportunity to train another member of the team.

Also it is my understanding is that the "priority:1-top" label designates that it is important enough to interrupt other scheduled work. Is that the intention here?

Anything that leaves PhET-iO sims in the main branch broken for 2 weeks seems like it justifies a "top" priority. Does that not seem important enough?

jbphet commented 11 months ago

Sorry to have broken things. I worked with @samreid on this, and started trying to write up a description for how to modify common code and handle the resulting phet-io API changes (see https://github.com/phetsims/phet-io/issues/1955#issuecomment-1672095895), but apparently there were some steps that we didn't complete. I'll follow up with him and see if we can get things fixed up.

samreid commented 11 months ago

We also ran into numerous other stale problem that occluded us from seeing and dealing with the recent changes. It will be much more effort to clean this up.

samreid commented 11 months ago

@zepumph and I have a meeting scheduled Monday to work on https://github.com/phetsims/phet-io/issues/1944. Let's put this work on hold until @zepumph and I have progress. Thanks @jbphet for being proactive about this!

jbphet commented 11 months ago

Some work has been done on migration rules recently that have fixed some of the problems that resulted from the changes to TextPushButton, see https://github.com/phetsims/joist/issues/934. In order to see what remained to be done, @samreid and I worked together today to run the migration wrapper test for the phet-io sims that were potentially impacted by the changes to TextPushButton. The list of potentially affected sims was derived from the commit that updated the API files after these changes were made, and the potentially affected sims are:

The greenhouse-effect sim is not included on this list because it hasn't been fully published with phet-io yet, so no migration rules are necessary.

All of these passed the migration wrapper test with the exception of gravity-and-orbits, and the problems revealed by that test did not appear to be related to the TextPushButton changes, so I think there is no additional work to be done for this particular issue. Closing.