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

Not all public query parameters are marked as such #974

Open
zepumph opened this issue Aug 26, 2020 · 2 comments
Open

Not all public query parameters are marked as such #974

zepumph opened this issue Aug 26, 2020 · 2 comments

Comments

@zepumph
Copy link
Member

zepumph commented Aug 26, 2020

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

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.

@chrisklus chrisklus assigned zepumph and unassigned chrisklus Aug 26, 2020
@arouinfar
Copy link

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.

@zepumph zepumph removed their assignment Mar 3, 2023
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

3 participants