Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
pixelzoom opened this issue Mar 7, 2017 · 9 comments
Closed

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

pixelzoom opened this issue Mar 7, 2017 · 9 comments
Assignees

Comments

@pixelzoom
Copy link
Contributor

pixelzoom commented Mar 7, 2017

This is the same problem as phetsims/molecule-shapes#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
Copy link
Contributor Author

phetsims/joist#406

@jbphet
Copy link
Contributor

jbphet commented Mar 8, 2017

The issue referenced in the comment immediately above (phetsims/joist#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
Copy link
Member

samreid commented Mar 9, 2017

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 phetsims/joist#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 phetsims/joist#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
Copy link
Contributor Author

@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
Copy link
Member

samreid commented Mar 9, 2017

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
Copy link
Contributor Author

@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 phetsims/arithmetic#179 (comment).

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
Copy link
Member

samreid commented Mar 9, 2017

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

@pixelzoom
Copy link
Contributor Author

See phetsims/joist#411 for how we can arrive at a solution to this and related issues.

@pixelzoom
Copy link
Contributor Author

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 phetsims/phetcommon#38.
Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants