Convert { min: , max: } => Range

[chrisklus]

@samreid and I noticed that many places use a range object literal instead of the Range type, and RangeIO currently only supports the Range type. Instead of loosening restrictions on RangeIO, we would prefer if Range was used from now on. Alternatively, we could add support for the object literal range in RangeIO.

We discovered this while working on https://github.com/phetsims/graphing-quadratics/issues/14, and the problem was introduced from changes to Range.js in https://github.com/phetsims/dot/issues/77

[chrisklus]

@zepumph and I added a fix to NumberProperty.js that converted options.range to a Range if an object literal range was used.

@samreid do you think this is an appropriate fix for now until we convert all object literal ranges to Range at some point? We would need to remove support for object literals in NumberProperty as well in order to support RangeIO, but @zepumph's current fix supports both types for NumberProperty.

[samreid]

This seems like a reasonable short-term workaround as I'm expecting our clients usages of options.range aren't doing anything crazy. For instance, here's a "crazy" case that could lead to unexpected behavior.

var range = {min:6,max:8};
var options = {range:range};
var p = new NumberProperty(7,options);
if (options.range!==range){
 console.log('something happened!');
}

Also, we should make a note to remove this workaround and replace it with an assertion once we've converted all sites.

[zepumph]

I added some doc that is good to have as long as we are using this workaround.

[chrisklus]

Beast of a change! Assigning to @samreid and @zepumph for review however you see fit, whoever gets to it first.

[chrisklus]

Marking for dev meeting 10/25/18 to notify devs that we should use Range instead of object literal ranges from now on.

[zepumph]

To review I searched for many versions of { min: .*, max:, in different orders, different spacing, etc. I couldn't find any more usages.

I then looked into the about 10 of the above commits, and they all looked thorough and complete to me.

[zepumph]

I see an issue from CT in optics lab that may have to do with this:

optics-lab : xss-fuzz : load
Uncaught Error: Assertion failed: max must be >= min. min: -100, max: -800
Error: Assertion failed: max must be >= min. min: -100, max: -800
    at window.assertions.assertFunction (https://bayes.colorado.edu/continuous-testing/snapshot-1540492864689/assert/js/assert.js:22:13)
    at new Range (https://bayes.colorado.edu/continuous-testing/snapshot-1540492864689/dot/js/Range.js?bust=1540496699429:22:15)
    at new ControlPanel (https://bayes.colorado.edu/continuous-testing/snapshot-1540492864689/optics-lab/js/optics-lab/view/ControlPanel.js?bust=1540496699429:159:70)
    at new ControlPanelManager (https://bayes.colorado.edu/continuous-testing/snapshot-1540492864689/optics-lab/js/optics-lab/view/ControlPanelManager.js?bust=1540496699429:68:29)
    at new OpticsLabScreenView (https://bayes.colorado.edu/continuous-testing/snapshot-1540492864689/optics-lab/js/optics-lab/view/OpticsLabScreenView.js?bust=1540496699429:42:32)
    at OpticsLabScreen.Screen.call.backgroundColorProperty [as createView] (https://bayes.colorado.edu/continuous-testing/snapshot-1540492864689/optics-lab/js/optics-lab/OpticsLabScreen.js?bust=1540496699429:26:34)
    at OpticsLabScreen.initializeView (https://bayes.colorado.edu/continuous-testing/snapshot-1540492864689/joist/js/Screen.js?bust=1540496699429:251:25)
    at Array.<anonymous> (https://bayes.colorado.edu/continuous-testing/snapshot-1540492864689/joist/js/Sim.js?bust=1540496699429:783:18)
    at https://bayes.colorado.edu/continuous-testing/snapshot-1540492864689/joist/js/Sim.js?bust=1540496699429:791:27
id: Bayes Chrome
Approximately 10/25/2018, 10:41:04 AM
optics-lab : xss-fuzz : run
Uncaught Error: Assertion failed: max must be >= min. min: -100, max: -800
Error: Assertion failed: max must be >= min. min: -100, max: -800
    at window.assertions.assertFunction (https://bayes.colorado.edu/continuous-testing/snapshot-1540492864689/assert/js/assert.js:22:13)
    at new Range (https://bayes.colorado.edu/continuous-testing/snapshot-1540492864689/dot/js/Range.js?bust=1540496699429:22:15)
    at new ControlPanel (https://bayes.colorado.edu/continuous-testing/snapshot-1540492864689/optics-lab/js/optics-lab/view/ControlPanel.js?bust=1540496699429:159:70)
    at new ControlPanelManager (https://bayes.colorado.edu/continuous-testing/snapshot-1540492864689/optics-lab/js/optics-lab/view/ControlPanelManager.js?bust=1540496699429:68:29)
    at new OpticsLabScreenView (https://bayes.colorado.edu/continuous-testing/snapshot-1540492864689/optics-lab/js/optics-lab/view/OpticsLabScreenView.js?bust=1540496699429:42:32)
    at OpticsLabScreen.Screen.call.backgroundColorProperty [as createView] (https://bayes.colorado.edu/continuous-testing/snapshot-1540492864689/optics-lab/js/optics-lab/OpticsLabScreen.js?bust=1540496699429:26:34)
    at OpticsLabScreen.initializeView (https://bayes.colorado.edu/continuous-testing/snapshot-1540492864689/joist/js/Screen.js?bust=1540496699429:251:25)
    at Array.<anonymous> (https://bayes.colorado.edu/continuous-testing/snapshot-1540492864689/joist/js/Sim.js?bust=1540496699429:783:18)
    at https://bayes.colorado.edu/continuous-testing/snapshot-1540492864689/joist/js/Sim.js?bust=1540496699429:791:27
id: Bayes Chrome
Approximately 10/25/2018, 10:41:04 AM

[chrisklus]

@zepumph that was the one invalid range I found, see https://github.com/phetsims/optics-lab/issues/22. Do you think anything else needs to be addressed for this issue?

[zepumph]

I just saw another issue. There were some Range imports that made their way into the ifphetio block instead of into the normal module blocks. For example in NodeIO.

[chrisklus]

Ahh shoot, I was trying to be wary of that once I noticed ifphetio blocks, but not soon enough apparently. Should I verify them all or merge the effort with your approach in https://github.com/phetsims/phet-io/issues/1396#issuecomment-432834556.

[zepumph]

I think that is likely best.

[zepumph]

Please note also that https://github.com/phetsims/tasks/issues/963 is a work in progress.

[chrisklus]

I think that is likely best.

Cool, thanks. I'd be surprised if there were more than one or two other cases of this, if any.

[chrisklus]

Since it was easy to scan through all of the // ifphetio blocks, I quickly verified that no other Range imports wound up there. Closing.

[chrisklus]

Regarding https://github.com/phetsims/dot/issues/80#issuecomment-432912643, we ended up having this discussion during 11/1/18 dev meeting, so devs are on board with using Range instead of the object literal range. We also clarified that this only applies to object literals with only min and max. For example, { min: , max: , color: } is still valid.