phetsims / neuron

"Neuron" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
1 stars 3 forks source link

phet.joist.sim.version is 'joist internal' #134

Closed pixelzoom closed 7 years ago

pixelzoom commented 7 years ago

This is the same problem as https://github.com/phetsims/molecule-shapes/issues/147, probably code that was copied from Molecule Shapes.

ParticlesWebGLNode should not be accessing phet.joist.sim.version, it is @public (joist internal).

      if ( document.domain === 'phet.colorado.edu' ) {
        window._gaq && window._gaq.push( [ '_trackEvent', 'WebGL Context Loss', 'neuron' + phet.joist.sim.version, document.URL ] );
      }

Assigning to @jbphet because he's primary developer, @jonathanolson because this code looks like it originated in Molecule Shapes.

pixelzoom commented 7 years ago

https://github.com/phetsims/joist/issues/406

jbphet commented 7 years ago

The issue referenced in the comment immediately above (https://github.com/phetsims/joist/issues/406) is about determining whether the version of the sim that is running is an RC or DEV version, but the code that is being questioned in this issue is trying to get the entire version. If we are not supposed to access phet.joist.sim.version, what is the accepted way to retrieve version information?

samreid commented 7 years ago

In

window._gaq && window._gaq.push( [ '_trackEvent', 'WebGL Context Loss', 'neuron' + phet.joist.sim.version, document.URL ] );

I wonder if that code is even necessary or can be superseded by https://github.com/phetsims/joist/issues/410

And similar code is in Molecule Shapes, it seems it should be factored out somehow.

Also, google-analytics.js is using 'version=' + encodeURIComponent( phet.chipper.version ) + '&' + instead of phet.joist.sim.version. Why?

Also, the 3rd usage of joist.sim.version is in this code:

      if ( ArithmeticQueryParameters.autoAnswer && window.phet.joist.sim.version.indexOf( '-' ) > 0 ) {
        this.autoAnswer();
      }

It seems like that code should be superseded by https://github.com/phetsims/joist/issues/406

I'm not opposed to joist.sim.version being public, but it doesn't seem appropriate for the 3 sites that are using it now.

pixelzoom commented 7 years ago

@jbphet asked:

what is the accepted way to retrieve version information?

There are currently 3 ways to get the version id:

(1) Violate the visibility annotations and use phet.joist.sim.version.

(2) Get it yourself from package.json:

var packageJSON = require( 'JOIST/packageJSON' );
var version = packageJSON.version;

(3) Get it yourself from SimVersion (if you only need some part of the version id):

var packageJSON = require( 'JOIST/packageJSON' );
var simVersion = SimVersion.parse( packageJSON.version, phet.chipper.buildTimestamp );

Imo, none of the above is acceptable. The problem is that handling of version id is currently a mess.

samreid commented 7 years ago

There are currently 3 ways to get the version id:

(4) I saw this code in google-analytics.js; it seems like phet.chipper.version is another way to get this information

  var pingParams = 'pingver=3&' +
                   'project=' + encodeURIComponent( phet.chipper.project ) + '&' +
                   'brand=' + encodeURIComponent( phet.chipper.brand ) + '&' +
                   'version=' + encodeURIComponent( phet.chipper.version ) + '&' +
pixelzoom commented 7 years ago

@samreid said:

I wonder if that code is even necessary or can be superseded by phetsims/joist#410

That is indeed the case in Arithmetic, see https://github.com/phetsims/arithmetic/issues/179#issuecomment-284894474.

But Neuron and Molecules Shapes need the version identifier. phetsims/joist#140 detects the type of the version identifier ("dev or rc"), it does not give you the version identifier.

samreid commented 7 years ago

@jonathanolson proposed a more general error reporting scheme in https://github.com/phetsims/joist/issues/410 that seems like the most appropriate way to report the webgl errors too.

pixelzoom commented 7 years ago

See https://github.com/phetsims/joist/issues/411 for how we can arrive at a solution to this and related issues.

pixelzoom commented 7 years ago

3/9/17 dev meeting: Molecule Shape and Neuron use this same bit of "context loss listener" code. Factor that out (and address the version identifier issue) in https://github.com/phetsims/phetcommon/issues/38. Closing.