phetsims / chipper

Tools for developing and building PhET interactive simulations.
MIT License
11 stars 14 forks source link

Not all public query parameters are marked as such #974

Open zepumph opened 4 years ago

zepumph commented 4 years ago

Over in https://github.com/phetsims/joist/issues/654, I saw that ?locale and ?colorProfile are advertised to PhET-iO clients, but they aren't marked as public:true in initialize globals.

When I mark them as such, I don't get the out-of-the-box error dialog that I would expect, even though they both already have default values.

Here is the patch:

Index: js/initialize-globals.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/initialize-globals.js    (revision 29b616a083a386ecb9744a5c55caccef1f2ab34d)
+++ js/initialize-globals.js    (date 1598466801137)
@@ -113,7 +113,8 @@
      */
     colorProfile: {
       type: 'string',
-      defaultValue: 'default'
+      defaultValue: 'default',
+      public: true
     },

     // Output deprecation warnings via console.warn, see https://github.com/phetsims/chipper/issues/882. For internal
@@ -260,7 +261,8 @@
      */
     locale: {
       type: 'string',
-      defaultValue: 'en'
+      defaultValue: 'en',
+      public: true
     },

     /**

This link makes a dialog about incorrect parameters: http://localhost:8080/friction/friction_en.html?brand=phet-io&phetioStandalone&screens=43

This link does not: http://localhost:8080/friction/friction_en.html?brand=phet-io&phetioStandalone&colorProfile=43

@chrisklus can you help me out?

chrisklus commented 4 years ago

As I was suspecting when we discussed via slack, the actual validation in context of the sim for colorProfile in your test (and I'm guessing locale too) doesn't happen in QSM. Both are just marked as type 'string' in initialize-globals.js, and QSM doesn't do any type validation for strings because it considers all input as a string. It currently does not try to parse string input say, as a number, and then fail if it succeeds.

In your test above, Friction does not appear to use ColorProfile.js, so that explains why you're not getting any dialog or sim errors. When I ran your test input on a sim that does use ColorProfile.js, it errored out from line 82 of ColorProfile.js:

// Query parameter may override the default profile name.
const initialProfileName = phet.chipper.queryParameters.colorProfile || ColorProfile.DEFAULT_COLOR_PROFILE_NAME;
if ( profileNames.indexOf( initialProfileName ) === -1 ) {
  throw new Error( `invalid colorProfile: ${initialProfileName}` );
}

For screens, we did something like this to support the error dialog for sim-specific validation:

// Query parameter may override the default profile name.
let initialProfileName = phet.chipper.queryParameters.colorProfile;
if ( profileNames.indexOf( initialProfileName ) === -1 ) {
  const errorMessage = `invalid colorProfile: ${initialProfileName}`;
  QueryStringMachine.addWarning( 'colorProfile', initialProfileName, errorMessage ); // public query parameters get warnings instead of errors
  assert && assert( false, errorMessage ); // for running tests or development, if desired
  initialProfileName = ColorProfile.DEFAULT_COLOR_PROFILE_NAME;
}

Tested with Ratio and Proportion.

arouinfar commented 4 years ago

In your test above, Friction does not appear to use ColorProfile.js, so that explains why you're not getting any dialog or sim errors.

I only include the colorProfile query parameter in guides for sims that have a projector mode, so it is highly unlikely that a client would try it out in Friction.