Add together API support

[samreid]

Concentration has been selected as a simulation to use for API/arch/metacog/together integration. This issue will track commits done in the concentration repo and in the beers-law-lab repo, so they can be reviewed when complete. Work will be done in a branch api until it can be reviewed by the primary sim developer, @pixelzoom.

[samreid]

@pixelzoom please let me know when we can review the changes above together. I'd like to move to master sooner rather than later, but only after we have had a chance to discuss and evaluate the changes.

[pixelzoom]

I'll have some time on Monday 3/9.

[samreid]

I added a way to test save/load dynamically at http://localhost:8080/together/examples/mirror.html (needs to have URLs updated to match your configuration). This loads from one iframe and plays back immediately to another iframe so we can easily visualize what is getting saved/restored from the state.

[samreid]

The above commit is a nice concise example of how to instrument a single feature in the simulation.

[samreid]

One thought: after working on this for a bit, I'm realizing that concentration-api.jsi and and concentration-api-routes.js should probably be defined in beers-law-lab/ instead concentration/

[samreid]

Yes, it should be moved, so things will still be instrumented when running beers-law-lab (which includes the concentration screen). But that means we'll need a different way to distributed these files (cannot just point to ${simname}/${simname}-api.js

[samreid]

I think I'll leave everything where it is until @pixelzoom & I have a chance to review it, so that viewing history, issue comments, etc. will be easy.

[pixelzoom]

If possible, please rename the api branch to something more descriptive. See phetsims/together#11.

[pixelzoom]

@samreid Adding this support requires promoting many things to public fields, e.g. n ConcentrationView:

- var soluteControls = new SoluteControls(...
+ this.soluteControls = new SoluteControls(...

As we discussed a couple of weeks ago, this is a necessary evil of this approach, we don't see any way around it, and the negative aspects should be addressed through documenting that this fields are for together only. So I'm surprised to see that there are no annotations or comments to indicate that these new fields are not intended to be part of the public interface, but are for use by together. And you've been doing this in both sim code and common code.

Recommended to adopt a convention immediately, use that convention going forward, and fix all sites where you've done this immediately. As the developer of Concentration, this is a prerequisite to merging this code into master.

[samreid]

As discussed with @pixelzoom today, we are going to investigate an alternative approach. I'll create a new issue and a new branch.

[samreid]

Recommended to adopt a convention immediately, use that convention going forward, and fix all sites where you've done this immediately.

We have abandoned this methodology in the concentration branch, and the only usage site I see in common code was in EyeDropperNode, and I moved the instance var back to closure var in the above commit. Anything else to do for your above comment https://github.com/phetsims/concentration/issues/34#issuecomment-77875918 ?

[samreid]

If possible, please rename the api branch to something more descriptive.

I deleted the api branches and new work is being done in the together branch. Anything else to do for https://github.com/phetsims/concentration/issues/34#issuecomment-77762998 ?

[pixelzoom]

@samreid asked:

Anything else to do for #34 (comment) ?

No.

[samreid]

I'm ready for assistance from @pixelzoom to discuss this branch. Please let me know when you are available.

[pixelzoom]

@samreid and I spent 30 minutes looking at this on Skype. Looking good.

I had a few recommendations (feel free to move these elsewhere, if that makes sense.)

(1) Change 'parent' to 'supertype' in type descriptions (TogetherTypes), since this is describing single-inheritance, and 'type' is used for describing the types for componentIDs.

(2) Move together type descriptions (currently in joist.TogetherTypes) closer to the actual JavaScript types that they are describing. The current (centralized) approach creates circular dependencies on joist in common code repos, and makes it likely that we'll forget to change something. For example, if I'm working on scenery-phet.FaucetNode, I'd like to make changes there, rather than remember to look for type 'Faucet' in joist.TogetherTypes. This would also facilitate keeping enumeration of legal values (e.g., values: [ 'liquid', 'solid' ]) in one place.

(3) For together types that have associated built-in JS types (String, Boolean,...), perhaps define those types in something like phet-core.TogetherTypes.

(4) Put the logic for coercing type values to/from strings in the type specifications. Coercion is currently hardcoded into together.js, and that approach doesn't scale the same way that the type specification mechanism does. Eg, each type defines a toString and fromString function.

[pixelzoom]

Assigning back to @samreid. Let me know when you're ready to discuss again.

[samreid]

I'm concerned about name collisions. For instance if we have a together.js type Object, and import it, then it will shadow window.Object and wreak havoc unexpectedly. Same issue to a lesser extend with String, Number, Boolean, etc. I'd rather not water down the public API or introduce inconsistencies like different casing for primitives vs composites. I guess right now I'm leaning toward using the names we want in the public API and dealing with the consequences in our code (like different names for imported types) as they come up.

[samreid]

(3) For together types that have associated built-in JS types (String, Boolean,...), perhaps define those types in something like phet-core.TogetherTypes.

I'm leaning toward using one file per type so it will be uniform across types instead of treating primitives as a special case.

[samreid]

(2) Move together type descriptions (currently in joist.TogetherTypes) closer to the actual JavaScript types that they are describing. The current (centralized) approach creates circular dependencies on joist in common code repos, and makes it likely that we'll forget to change something. For example, if I'm working on scenery-phet.FaucetNode, I'd like to make changes there, rather than remember to look for type 'Faucet' in joist.TogetherTypes. This would also facilitate keeping enumeration of legal values (e.g., values: [ 'liquid', 'solid' ]) in one place.

Everyone please be aware this means Scenery, Dot, Axon, etc will be getting these type definitions.

[samreid]

(2) Move together type descriptions (currently in joist.TogetherTypes) closer to the actual JavaScript types that they are describing.

I have defined the following type in TogetherTypes.js

  var Button = {
    name: 'Button',
    superType: Node,
    events: {
      fired: 'fired'
    }
  };

Note that this does not apply to a single Button definition in the code (in Sun, for instance) but pertains to several different actual implementations. In my opinion, it is essential for us to simplify our complex type hierarchy before presenting to 3rd parties/researchers--but where should the declaration of Button live? And how will we handle similar cases where the implementation Type doesn't match what we want to expose as the public API type?

[samreid]

Also, the way the require statements are looking now is like this:

  var Object = require( 'PHET_CORE/together/Object' ).TogetherType;
  var Node = require( 'SCENERY/nodes/Node' ).TogetherType;

However, this is unconventional style of imports but works well to avoid name collisions between Node (the scenery type) and Node (the together type). The API declaration is supposed to be human readable, so its important that the types used in the file are the names we want and not some awkward renaming to avoid collisions.

[samreid]

It would be best to discuss the above issues before I put a lot of work into this. Feel free to respond asynchronously (if the above comments are sufficiently clear) or let me know when you are available for discussion @pixelzoom.

[samreid]

Reassigning to @pixelzoom since I am ready for discussion.

[pixelzoom]

After this morning's discussion with @samreid...

For type definitions, I'm leaning towards either: (1) one TogetherTypes file per repo, or (2) everything in a single together.TogetherTypes file. For (2), my main concern would be how large that file might get.

We also discussed the possibility of having all of the *-together-api.js files live in the together repo, see https://github.com/phetsims/together/issues/11#issuecomment-85088984.

[samreid]

@pixelzoom what do you think about starting with (2) and if/when the file gets too large then going to (1) one file per repo, but keeping all of those files in the together repo. For instance: together/js/api/Joist.js etc.

[pixelzoom]

@samreid That sounds fine.

[pixelzoom]

@samreid asked via Skype:

what else left to do before moving beers-law-lab together branch to master?

Push stuff in BLLDropperNode into scenery-phet.EyeDropperNode, as described in https://github.com/phetsims/arch/issues/14#issuecomment-85135586. Or are you avoiding that because calling together.addComponent before the BLLDropperNode is fully constructed will cause problems? If so, then that would be a huge general problem.

[samreid]

The current customization scheme doesn't handle subclassing well, but that shouldn't be solved at the moment. I moved componentID and together.addComponent to scenery-phet in the above two commits. Next I'll merge beers-law-lab together to master. Note that no merge is necessary for concentration because it no longer has any code (aside from added entries to the html file).

[samreid]

I merged together to master and deleted together and arch branches. @pixelzoom I'm ready to close this issue if you are satisfied.

[pixelzoom]

I reviewed the merge. A couple of questions...

(1) beers-law-lab.Dropper contains this dead code, can it be deleted?

55 //together && together.addComponent( 'concentrationScreen.dropper.location', thisDropper.locationProperty );

(2) beers-law-lab.SoluteComboBox contains this line:

49 listNodeVisiblePropertyID: 'concentrationScreen.soluteComboBox.listVisible'

ComboBox has no such option, and 'PropertyID' doesn't match the 'ComponentID' naming convention. I found it by accident because buttonNodeComponentID is on the line above it. Is this vestigial?

[pixelzoom]

I like how the together-specific API and html files live in the together repo.

[pixelzoom]

@samreid concentration still has a 'together' branch.

[samreid]

I addressed the dead and unused code issues raised in https://github.com/phetsims/concentration/issues/34#issuecomment-86187723

The together branch of concentration just contains the additional scripts in the _en.html file that are required to launch. I think we shall keep that branch for all sims that can are instrumented for together.

@pixelzoom I'm ready to close this issue if you are satisfied.

[pixelzoom]

Reopening, I discovered some things that I didn't find when searching for 'together' and 'componentID'. In ConcentrationModel:

    // The Public API for this simulation provides a simplified name for the selected solute
    // Here, we perform 2-way binding between the solute name and the solute instance
    // This means we can get the value of the solute by name, and set it back by name,
    // which enables save/load/record/playback/configuration through query parameters/etc.
    var soluteAPINameProperty = new Property( thisModel.solute.value.apiName, { componentID: 'concentrationScreen.solute' } );
    soluteAPINameProperty.link( function( soluteAPIName ) {
      for ( var i = 0; i < thisModel.solutes.length; i++ ) {
        var solute = thisModel.solutes[ i ];
        if ( solute.apiName === soluteAPIName ) {
          thisModel.solute.value = solute;
        }
      }
    } );
    thisModel.solute.link( function( solute ) {
      soluteAPINameProperty.value = solute.apiName;
    } );

(1) Describing this as "The Public API" tells the reader nothing. The public API for a JavaScript type is all of the properties that are publicly accessible. Describe this a "the together API" or something specific.

(2) I'm a little concerned about having to set up this 2-way binding. This was not in the "4 steps" that you've enumerated many times. And it's something that will need to be done whenever there is selection. Let's discuss. Or at least make other developers aware of this additional instrumentation requirement.

[pixelzoom]

The above observer that handles the 2-way binding is also not robust. If solute.apiName !== soluteAPIName for all solutes, that is a programming error and it's silently ignore.

[pixelzoom]

I have the same criticisms about this in Solute's constructor:

@param {string} apiName - A non-internationalized unique identifier that can be used to access the solute

Together is not the only API, this needs to say something about together specifically.

[pixelzoom]

I also do not like the listItemComponentID (which is missing from jsdoc, btw) in Solute constructor. Example:

Solute.DRINK_MIX = new Solute(...,
  'drinkMix',
 'concentrationScreen.soluteComboBox.drinkMixButton',
...
);

The responsibility for naming combo box list items should live with ComboBox.  Also consider having ComboBox set up the 2-way binding between items and their componentIDs.

[pixelzoom]

Trying to write JSdoc for Solute's @param listItemComponentID shows how out-of-place it is. Eg:

@param {string} listItemComponentID - componentID for combo box list items

Why would anything about combo box be in the model? What if some other UI component were used to select the solute? What if there was >1 way to select the solute? This does not belong here.

[samreid]

This was not in the "4 steps" that you've enumerated many times.

The 4-step plan assumes the simulation is already implemented using idiomatic axon & PhET patterns. This will not be the case in many places and hence will require a significant amount of labor.

[pixelzoom]

Discussed alternatives to 2-way coupling and Solute.apiName/listItemComponentID with @samreid and @jbphet. Created "together-values" branch of beers-law-lab to investigate further, see https://github.com/phetsims/beers-law-lab/issues/92. Notes about how to address above concerns appear in https://github.com/phetsims/concentration/issues/38.

Closing.