zepumph
commented
4 years ago We found that public is the schema field that should be used for this, and is supposed to already be handling this. The issue is that it isn't totally supported for arrays, because the elements aren't treated as public. A quick hack to demonstrate the problem is the following:
- run a sim with
?screens=43289. Note the hard fail with a console error. - add this patch to initialize globals:
Index: js/initialize-globals.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- js/initialize-globals.js (revision beaa4fd18398d2842d46edf320089e194720f32c) +++ js/initialize-globals.js (date 1595438893689) @@ -417,7 +417,9 @@ screens: { type: 'array', elementSchema: { - type: 'number'
- type: 'number',
- public: true,
- defaultValue: 1 }, defaultValue: null, isValidValue: function( value ) {
* Rerun the sim with the same query parameter, and note that you get the invalid QP warning dialog now.
So the issue is that we aren't forwarding the `public` field into the element schema for array.
@samreid and I investigated and found a solution based on the current implementation of `public`. In this, we forward the `public` field to the element schema so that it can be graceful and apply warnings. This works at providing the dialog for `?screens=Micro` or `screens=Micro,Macro`. Committed above.
This work does not seem done yet though, as this doesn't cover the case `?screens=1.1`. While working on this we found that the handling for `public` seems very incomplete. We needed to add the below patch in order to successfully get element schemas to respect `isValidValue`. But even so this feels like the wrong way to approach this problem. The basic algorithm for QSM.getForString is (1) validateSchema, (2) parseValues into appropriate types, then (3) validate those values. We found that much of the warning infrastructure was build into the parsing, in `getValidValue`. So the above commit builds out that logic for parsing arrays' elements. We noticed that the validation step doesn't validate array elements at all:
` isValidValue: value => Array.isArray( value ) || value === null`.`
<details><summary>patch that adds isValidValue to parsing functions</summary>
```diff
Index: query-string-machine/js/QueryStringMachine.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- query-string-machine/js/QueryStringMachine.js (revision fce9acaca3dea7caa639454b052843b691b14193)
+++ query-string-machine/js/QueryStringMachine.js (date 1595443998256)
@@ -589,10 +589,22 @@
*/
const parseNumber = function( key, schema, string ) {
const number = Number( string );
- const ok = !isNaN( number );
+ const ok = !isNaN( number ) && ( schema.isValidValue ? schema.isValidValue( string ) : true );
return getValidValue( ok, key, ok ? number : string, schema, `value must be a number for key "${key}"` );
};
+ /**
+ * Parses the value for a type 'number'.
+ * @param {string} key - the query parameter name
+ * @param {Object} schema - schema that describes the query parameter, see QueryStringMachine.get
+ * @param {string|null} string - value from the query parameter string
+ * @returns {number}
+ */
+ const parseString = function( key, schema, string ) {
+ const ok = schema.isValidValue ? schema.isValidValue( string ) : true;
+ return getValidValue( ok, key, ok ? string : string, schema, `value must be a number for key "${key}"` );
+ };
+
/**
* Parses the value for a type 'array'.
* @param {string} key - the query parameter name
@@ -726,7 +738,7 @@
required: [],
optional: [ 'defaultValue', 'validValues', 'isValidValue', 'private', 'public' ],
validateSchema: null, // no type-specific schema validation
- parse: ( key, schema, string ) => string, // The variable to be parsed is already string, so it is guaranteed to parse as a string.
+ parse: parseString,
isValidValue: value => value === null || typeof value === 'string'
},
Index: chipper/js/initialize-globals.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- chipper/js/initialize-globals.js (revision beaa4fd18398d2842d46edf320089e194720f32c)
+++ chipper/js/initialize-globals.js (date 1595443998240)
@@ -417,7 +417,8 @@
screens: {
type: 'array',
elementSchema: {
- type: 'number'
+ type: 'number',
+ isValidValue: Number.isInteger
},
defaultValue: null,
isValidValue: function( value ) {
My recommendation is to not implement the above patch, and to instead build out an additional step in the isValidValue stage of getForString that iterates through element types and checks isValidValue on them.
@pixelzoom's comment over in https://github.com/phetsims/joist/issues/644#issuecomment-662588001 said it best, marking this as blocks publication:
screensis a public query parameter, so it's supposed to fail gracefully -- display a warning dialog and revert to defaults. It does not do that and fails hard for the 2 outstanding cases described in https://github.com/phetsims/joist/issues/644#issue-663325840. So if it were my call, I feel that this is still unacceptably buggy, and therefore still blocks publication. But I'll be happy to defer to a designer.
samreid
pixelzoom
chrisklus
arouinfar
Over in https://github.com/phetsims/joist/issues/644 there were two suggestions for being more graceful about how we support invalid parameters as it pertains to the production dialog that shows query parameter errors to users:
@samreid and I are currently brainstorming a couple of ways to tackle this problem. The main issue is that currently, the warnings only apply to parameters that pass the "valid" check in the QP schema, but for some other reason are invalid. Instead, this should be expanded to all values of query parameters in these cases, so that a full list of warnings can be shown.