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

Create a PhET-iO migration guide. #278

Closed samreid closed 3 years ago

samreid commented 3 years ago

While working on https://github.com/phetsims/natural-selection/issues/275, the output from the diff wrapper is too verbose, redundant and technical to be useful to a client hoping to update from 1.2 to 1.3. The diff wrapper has no way of knowing things like "we now use inputEnabledProperty instead of pickableProperty". Yet there are 263 lines like this in the breaking API diff:

PhET-iO Element missing: naturalSelection.labScreen.view.timeControlNode.pickableProperty

We have been talking about sharing the diff wrapper as a guide to help our clients migrate from 1.2 to 1.3, but the changes are so numerous and technical that I don't recommend it. Instead, I think we should prepare a hand-written "migration guide" using the diff wrapper to help us make sure we cover everything. The migration guide can be shared via email for now. In the future we may want a place to publish migration guides. It would be best and most efficient to commit and track the migration guide in the sim repo, however, I think it will need to be committed elsewhere for the same reason we commit client-guides and reference API files elsewhere. phet-io-client-guides seems like the most natural repo, since it a guide for clients to migrate, but I have no familiarity with that repo, so I'm not confident about that.

This work can be done in master and does not need to be published in the natural selection RC. I should also mention, I don't think this needs to be perfectly refined, but (a) the rough draft is already much better than the diff wrapper and (b) we can use this case to think through how we will want to solve this problem in the future.

I'll mark it for discussion Thursday, though @zepumph and I may have an opportunity to talk about it in advance on Wednesday.

pixelzoom commented 3 years ago

From https://github.com/phetsims/natural-selection/issues/275#issuecomment-828636620, below is the latest migration guide that @zepumph @samreid and I came up with for NS 1.3.

To conver Diff Wrapper "Breaking Changes" output to this format, we manually:

Questions to discuss:

Natural Selection 1.2 -> 1.3 Migration Guide

// model visibleProperty is now passed to view.
// See https://github.com/phetsims/natural-selection/issues/262
naturalSelection.introScreen.view.graphs.populationNode.populationGraphNode.dataProbeNode.visibleProperty.phetioTypeName changed from PropertyIO<BooleanIO> to LinkedElementIO
naturalSelection.labScreen.view.graphs.populationNode.populationGraphNode.dataProbeNode.visibleProperty.phetioTypeName changed from PropertyIO<BooleanIO> to LinkedElementIO

// Removed useless IOType, use parent instead.
// See https://github.com/phetsims/sun/issues/558
naturalSelection.introScreen.view.timeControlNode.fastForwardButton.phetioTypeName changed from RoundMomentaryButtonIO to NodeIO
naturalSelection.labScreen.view.timeControlNode.fastForwardButton.phetioTypeName changed from RoundMomentaryButtonIO to NodeIO

// Used to extend ReferenceIO but now uses IOType.supertype.
// See https://github.com/phetsims/tandem/issues/211
AlleleIO parameter types changed from ObjectIO to . This may or may not be a breaking change, but we are reporting it just in case.
GeneIO parameter types changed from ObjectIO to . This may or may not be a breaking change, but we are reporting it just in case.

// environmentPanel was added as a parent element.
// See https://github.com/phetsims/natural-selection/issues/263
naturalSelection.introScreen.view.environmentNode moved to naturalSelection.introScreen.view.environmentPanel.environmentNode
naturalSelection.introScreen.view.environmentRadioButtonGroup moved to naturalSelection.introScreen.view.environmentPanel.environmentRadioButtonGroup
naturalSelection.introScreen.view.generationClockNode moved to naturalSelection.introScreen.view.environmentPanel.generationClockNode
naturalSelection.introScreen.view.playButtonGroup moved to naturalSelection.introScreen.view.environmentPanel.playButtonGroup
naturalSelection.labScreen.view.environmentNode moved to naturalSelection.labScreen.view.environmentPanel.environmentNode
naturalSelection.labScreen.view.environmentRadioButtonGroup moved to naturalSelection.labScreen.view.environmentPanel.environmentRadioButtonGroup
naturalSelection.labScreen.view.generationClockNode moved to naturalSelection.labScreen.view.environmentPanel.generationClockNode
naturalSelection.labScreen.view.playButtonGroup moved to naturalSelection.labScreen.view.environmentPanel.playButtonGroup

// pickableProperty has been replaced with inputEnabledProperty. 
// See https://github.com/phetsims/scenery/issues/1158
. . . 

// opacityProperty has been uninstrumented.
// See https://github.com/phetsims/scenery/issues/1158
. . .

// PressListener pressAction and releaseAction became phetioReadOnly: true. 
// See https://github.com/phetsims/scenery/commit/3bf7cb79e5aeb72ef30055c28759d151c62c66b5
. . .

// ObservableArray events and methods were replaced with Emitters.
// See https://github.com/phetsims/axon/issues/330
ObservableArrayIO is missing event: itemAdded, please use elementAddedEmitter
ObservableArrayIO is missing event: itemRemoved, please use elementRemovedEmitter
Method missing, type=ObservableArrayIO, method=addItemAddedListener, please use elementAddedEmitter
Method missing, type=ObservableArrayIO, method=addItemRemovedListener, please use elementRemovedEmitter
Method missing, type=ObservableArrayIO, method=getLength, please use lengthProperty

// phetioEngine methods were converted to Emitters.
// See https://github.com/phetsims/phet-io/issues/1655
Method missing, type=PhetioEngineIO, method=addPhetioElementAddedListener
Method missing, type=PhetioEngineIO, method=addPhetioElementRemovedListener
Method missing, type=PhetioEngineIO, method=addPhetioElementsBaselineListener
Method missing, type=PhetioEngineIO, method=addTypesAddedListener
Method missing, type=PhetioEngineIO, method=endEvent
Method missing, type=PhetioEngineIO, method=startEvent

// Input.js mouseDownAction parameter signature was changed to support iframe fix.
// See https://github.com/phetsims/natural-selection/issues/264.
naturalSelection.general.controller.input.mouseDownAction.phetioTypeName changed from ActionIO<Vector2IO, EventIO> to ActionIO<NullableIO<NumberIO>, Vector2IO, EventIO> changed a parameter
zepumph commented 3 years ago

From PhET-iO meeting today,

KP: I can publish the above to a google doc and deliver it as needed with our clients.

If things stabilize more, then we will potentially be able to give the diff wrapper to clients, especially with the new tree-like format, but for now, these migration guides don't take too long to create from the list of breaking changes, and are much easier to read than the JSON diff patch output in the diff wrapper.

assigning to @kathy-phet to create the google doc.

@samreid what more do you want to discuss here?

samreid commented 3 years ago

If we do this more in the future, we will want to establish better patterns about:

(a) how the file is maintained in git (b) how the file can be published with a deployment

But for now, sharing with the client via a google doc seems appropriate.

kathy-phet commented 3 years ago

Leaving this open until I deliver this to the client.

kathy-phet commented 3 years ago

Made a google doc for now.

pixelzoom commented 3 years ago

@kathy-phet said:

Leaving this open until I deliver this to the client.

Made a google doc for now.

@kathy-phet and PhET-iO team (@samreid @zepumph):

zepumph commented 3 years ago

I have no idea where the google doc for the migration guide is.

Is this something that needs to be done for all sims going forward?

It is not clear to me, and I think we should discuss the importance of having a manual migration guide with each version, or if this isn't needed until some point in the future (more clients, clients have more code, clients move to production, etc).

Does it need to be updated for the 1.4 release?

I ran the same comparison between 1.4.0-rc.1 (on phet-dev), and the published 1.3 on phet-io.colorado.edu, and got this list of "Breaking" changes:

PhET-iO Element missing: naturalSelection.general.model.focusProperty
naturalSelection.global.model.alleles.brownFurAllele.phetioState changed from true to false
naturalSelection.global.model.alleles.floppyEarsAllele.phetioState changed from true to false
naturalSelection.global.model.alleles.longTeethAllele.phetioState changed from true to false
naturalSelection.global.model.alleles.shortTeethAllele.phetioState changed from true to false
naturalSelection.global.model.alleles.straightEarsAllele.phetioState changed from true to false
naturalSelection.global.model.alleles.whiteFurAllele.phetioState changed from true to false
naturalSelection.introScreen.model.genePool.earsGene.phetioState changed from true to false
naturalSelection.introScreen.model.genePool.furGene.phetioState changed from true to false
naturalSelection.introScreen.model.genePool.teethGene.phetioState changed from true to false
naturalSelection.labScreen.model.genePool.earsGene.phetioState changed from true to false
naturalSelection.labScreen.model.genePool.furGene.phetioState changed from true to false
naturalSelection.labScreen.model.genePool.teethGene.phetioState changed from true to false
DialogIO supertype changed from ReferenceIO<NodeIO> to DynamicMarkerIO. This may or may not be a breaking change, but we are reporting it just in case.
Method missing, type=PhetioEngineIO, method=getFullPhetioAPI
Type missing: ReferenceIO<NodeIO>

Here I annotated it:


#No worries here, as NS has no keyboard navigation support
PhET-iO Element missing: naturalSelection.general.model.focusProperty

# These didn't have any data before in state, just a phetioID, like this: 
# "naturalSelection.global.model.alleles.longTeethAllele": { "phetioID": "naturalSelection.global.model.alleles.longTeethAllele" },
naturalSelection.global.model.alleles.brownFurAllele.phetioState changed from true to false
naturalSelection.global.model.alleles.floppyEarsAllele.phetioState changed from true to false
naturalSelection.global.model.alleles.longTeethAllele.phetioState changed from true to false
naturalSelection.global.model.alleles.shortTeethAllele.phetioState changed from true to false
naturalSelection.global.model.alleles.straightEarsAllele.phetioState changed from true to false
naturalSelection.global.model.alleles.whiteFurAllele.phetioState changed from true to false
naturalSelection.introScreen.model.genePool.earsGene.phetioState changed from true to false
naturalSelection.introScreen.model.genePool.furGene.phetioState changed from true to false
naturalSelection.introScreen.model.genePool.teethGene.phetioState changed from true to false
naturalSelection.labScreen.model.genePool.earsGene.phetioState changed from true to false
naturalSelection.labScreen.model.genePool.furGene.phetioState changed from true to false
naturalSelection.labScreen.model.genePool.teethGene.phetioState changed from true to false

# A purposeful and good change, that shouldn't effect anyone
DialogIO supertype changed from ReferenceIO<NodeIO> to DynamicMarkerIO. This may or may not be a 
breaking change, but we are reporting it just in case.

# got rid of method that shouldn't have been used anyways by clients
Method missing, type=PhetioEngineIO, method=getFullPhetioAPI

#From DialogIO change
Type missing: ReferenceIO<NodeIO>

@kathy-phet, how should we proceed with this as a general process? Marking for PhET-iO meeting to check in on its progress.

pixelzoom commented 3 years ago

This looks like this is probably "the Google doc", @kathy-phet please confirm.

https://docs.google.com/document/d/1LM6g029X3dydA4LX8Auie1GxOfkb2Yqt8u9w9l2g-bc/edit

pixelzoom commented 3 years ago

If this is going to be a part of the general process, then highly recommended to:

(1) Put these files in GitHub, perhaps in phet-io/migration-guides/{{repo}}/. (2) Standardize file format (.txt, .md, .html?)
(3) Standardize file names, e.g. {{repo}}-{{oldVersion}}-{{newVersion}}-migration-guide.md (4) Add migration guide to doc, checklists, etc.

For example, files in phet-io/migration-guides/natural-selection/ might look like:

natural-selection-1.2-1.3-migration-guide.md
natural-selection-1.3-1.4-migration-guide.md
pixelzoom commented 3 years ago

From https://github.com/phetsims/phet-io/issues/1800#issue-940150387:

Do this for Natural Selection (not needed for GAO because it is the first release)

Blocking for NS 1.4. On hold until https://github.com/phetsims/phet-io/issues/1800 is completed.

zepumph commented 3 years ago

I still think this issue should be on hold until @arouinfar is able to create a template that NS can follow. But from a slack conversation, it didn't seem like this needed to hold up publication.

pixelzoom commented 3 years ago

@zepumph said:

... But from a slack conversation, it didn't seem like this needed to hold up publication.

From https://github.com/phetsims/phet-io/issues/1800#issuecomment-878422025

I'm restoring the "blocks-publication" label. Since the primary purpose of Natural Selection 1.4 is to provide migration support, I don’t see a point in proceding until this issue is resolved and solid. Cherry-picking a partial solution and patching later is not necessary or desirable.

zepumph commented 3 years ago

Since the primary purpose of Natural Selection 1.4 is to provide migration support,

This is not true. The primary purpose was to make sure that the sim is well set up to provide migration support in NS 1.5.

I don’t see a point in proceding until this issue is resolved and solid.

What about the argument that this seems to be the only issue holding up the next RC, and our goal for the quarter is to publish as many PhET-iO sims as possible? It has been weeks since the last RC. Wouldn't it be more efficient for the RC to go out without this supporting document, and patch it in later (with no changes to the sim or build process after cherry-picking https://github.com/phetsims/chipper/commit/d6184393ffb952f6e1e221a65993b6a9336fa4a6 and https://github.com/phetsims/phet-io-wrappers/commit/67eebb2fe2a7bcecfaab049b8517c27e9da002f5 (which are ready to go).

I would recommend spending our time getting sims out, and making sure that QA is consistently fully loaded with sims to test.

zepumph commented 3 years ago

During todays' PhET-iO Meeting we decided that on master, natural selection's migration guide will include a section on migrating from 1.2->1.3 and also 1.3->1.4. This doesn't need a fancy template or anything, it just needs the content from https://github.com/phetsims/natural-selection/issues/278#issuecomment-828703884 (in the google doc) and this part for 1.3->1.4:

#No worries here, as NS has no keyboard navigation support
PhET-iO Element missing: naturalSelection.general.model.focusProperty

# These didn't have any data before in state, just a phetioID, like this: 
# "naturalSelection.global.model.alleles.longTeethAllele": { "phetioID": "naturalSelection.global.model.alleles.longTeethAllele" },
naturalSelection.global.model.alleles.brownFurAllele.phetioState changed from true to false
naturalSelection.global.model.alleles.floppyEarsAllele.phetioState changed from true to false
naturalSelection.global.model.alleles.longTeethAllele.phetioState changed from true to false
naturalSelection.global.model.alleles.shortTeethAllele.phetioState changed from true to false
naturalSelection.global.model.alleles.straightEarsAllele.phetioState changed from true to false
naturalSelection.global.model.alleles.whiteFurAllele.phetioState changed from true to false
naturalSelection.introScreen.model.genePool.earsGene.phetioState changed from true to false
naturalSelection.introScreen.model.genePool.furGene.phetioState changed from true to false
naturalSelection.introScreen.model.genePool.teethGene.phetioState changed from true to false
naturalSelection.labScreen.model.genePool.earsGene.phetioState changed from true to false
naturalSelection.labScreen.model.genePool.furGene.phetioState changed from true to false
naturalSelection.labScreen.model.genePool.teethGene.phetioState changed from true to false

# A purposeful and good change, that shouldn't effect anyone
DialogIO supertype changed from ReferenceIO<NodeIO> to DynamicMarkerIO. This may or may not be a 
breaking change, but we are reporting it just in case.

# got rid of method that shouldn't have been used anyways by clients
Method missing, type=PhetioEngineIO, method=getFullPhetioAPI

#From DialogIO change
Type missing: ReferenceIO<NodeIO>

But improved.

zepumph commented 3 years ago

Basic migration guide added for NS above. Please cherry-pick

https://github.com/phetsims/phet-io-client-guides/commit/55b1ddc477814c66843e01d1a63eff668138476e https://github.com/phetsims/phet-io-wrappers/commit/67eebb2fe2a7bcecfaab049b8517c27e9da002f5 https://github.com/phetsims/chipper/commit/d6184393ffb952f6e1e221a65993b6a9336fa4a6

To test: checkout shas, build the sim, and make sure that the migration guide is accessible from the wrapper index (in the last row).

zepumph commented 3 years ago

This is what it looks like on build in master:

image

samreid commented 3 years ago

Looks great! I added a 👍 to the prior comment, but wanted to add a 👍 here for visibility!

pixelzoom commented 3 years ago

I'm getting merge conflicts with https://github.com/phetsims/phet-io-wrappers/commit/67eebb2fe2a7bcecfaab049b8517c27e9da002f5. And I thought we decided in today's meeting that @zepumph was going to patch NS 1.4.

pixelzoom commented 3 years ago

Oh wait, let me try again. I might have forgotten to git checkout natural-selection-1.4.

pixelzoom commented 3 years ago

Adding one more commit to the cherry-pick: https://github.com/phetsims/phet-io-client-guides/commit/fcb38dcb81e2658f61323d61ee5981465a04dcae. This fixed several grammar/punctuation errors, distracting and inappropriate in a public-facing document. And it uses a consistent '//' token for comment.

The complete set of shas to cherry-pick is now:

https://github.com/phetsims/phet-io-client-guides/commit/55b1ddc477814c66843e01d1a63eff668138476e https://github.com/phetsims/phet-io-client-guides/commit/fcb38dcb81e2658f61323d61ee5981465a04dcae https://github.com/phetsims/phet-io-wrappers/commit/67eebb2fe2a7bcecfaab049b8517c27e9da002f5 https://github.com/phetsims/chipper/commit/d6184393ffb952f6e1e221a65993b6a9336fa4a6

pixelzoom commented 3 years ago

I'm still unable to cherry-pick https://github.com/phetsims/phet-io-wrappers/commit/67eebb2fe2a7bcecfaab049b8517c27e9da002f5:

% grunt checkout-target --repo=natural-selection --target=1.4
% cd phet-io-client-guides
% git checkout -b natural-selection-1.4
% git cherry-pick 55b1ddc477814c66843e01d1a63eff668138476e
% cd phet-io-wrappers
% git checkout natural-selection-1.4
% git cherry-pick 67eebb2fe2a7bcecfaab049b8517c27e9da002f5
error: could not apply 67eebb2... add migration guide to the wrapper index, https://github.com/phetsims/phet-io/issues/1800
hint: after resolving the conflicts, mark the corrected paths
hint: with 'git add <paths>' or 'git rm <paths>'
hint: and commit the result with 'git commit'
pixelzoom commented 3 years ago

Continuing to investigate the conflict:

% git status
On branch natural-selection-1.4
Your branch is up to date with 'origin/natural-selection-1.4'.

You are currently cherry-picking commit 67eebb2.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Changes to be committed:

    modified:   index/index.html

Unmerged paths:
  (use "git add <file>..." to mark resolution)

    both modified:   index/index.js

Opening index/index.js with vi, the conflict is identified by "<<<<<<< HEAD":

<<<<<<< HEAD
  'use strict';
=======

>>>>>>> 67eebb2... add migration guide to the wrapper index, https://github.com/phetsims/phet-io/issues/1800

It looks like "use strict" was removed in https://github.com/phetsims/phet-io-wrappers/commit/361a103ffac5d317d28bd9077fe8019cad72ffc1. Do I need to cherry-pick that commit too? Please advise.

pixelzoom commented 3 years ago

I got on a Zoom call with @samreid. He was familiar with https://github.com/phetsims/phet-io-wrappers/commit/361a103ffac5d317d28bd9077fe8019cad72ffc1, and we added it to the list of shas to cherry-pick.

Tested and patched in NS 1.4, ready for next RC test.

To verify this issue:

  1. Navigate to the Wapper index.
  2. Click on the "Migration Guide" link (bottom of the Link column)
  3. Verify that the Migration Guide looks like this:

screenshot_242

zepumph commented 3 years ago

And I thought we decided in today's meeting that @zepumph was going to patch NS 1.4.

Yes I remember now! I'm terribly sorry for the short term amnesia yesterday after meetings, and thanks for taking care of it to get the next RC out.

KatieWoe commented 3 years ago

The wrapper index for rc.2 now has a link to Migration Guide. It works, is password protected and looks good.