Why do schemas 'string' and 'array' allow `null` as a value?

[zepumph]

This is the isValidValue for the string type:

isValidValue: value => value === null || typeof value === 'string'

To me it seems like if a value isn't provided for string, it should be an empty string, like ''. This isn't blocking any work, but just seemed strange to me. It isn't clear if any code on our project is depending on this weirdness (I hope not).

[jonathanolson]

This came up when I typed QueryStringMachine for https://github.com/phetsims/chipper/issues/1253. It is used as null in a number of locations, so I ended up typing it that way. Lots of reliance on null here.

[pixelzoom]

It's not just the 'string' schema type that's odd in QueryStringMachine. 'array' has the same null quirk:

isValidValue: value => Array.isArray( value ) || value === null

I've definitely run into this before, in weird head-scratching situations. I think we should investigate, and (maybe) change it.

[zepumph]

We discussed this at dev meeting today.

In GQ, we took a look at equationsBackgroundColor and saw that both ?equationsBackgroundColor and ?equationsBackgroundColor= returned null. Even though this is a string type parameter. @pixelzoom volunteered to take a look at this. We potentially decided it would be nice to have ?equationsBackgroundColor throw an error, and ?equationsBackgroundColor= return an empty string from getValues. Good luck!

[pixelzoom]

Note that null is also allowed by the QueryStringMachineSchema ambient type in phet-types.d.ts:

declare type QueryStringMachineSchema = {
...
  {
    type: 'string';
    defaultValue?: string | null;
    validValues?: readonly ( string | null )[];
    isValidValue?: ( n: string | null ) => boolean;
  } |
  {
    type: 'array';
    elementSchema: QueryStringMachineSchema;
    separator?: string;
    defaultValue?: null | readonly IntentionalAny[];
    validValues?: readonly IntentionalAny[][];
    isValidValue?: ( n: IntentionalAny[] ) => boolean;
  } 

[pixelzoom]

Here's the code that's responsible for the problem that @zepumph described in https://github.com/phetsims/query-string-machine/issues/43#issuecomment-1205622086:

    const getValues = function( key, string ) {
      const values = [];
      const params = string.slice( 1 ).split( '&' );
      for ( let i = 0; i < params.length; i++ ) {
        const splitByEquals = params[ i ].split( '=' );
        const name = splitByEquals[ 0 ];
        const value = splitByEquals.slice( 1 ).join( '=' ); // Support arbitrary number of '=' in the value
        if ( name === key ) {
          if ( value ) {
            values.push( decodeURIComponent( value ) );
          }
          else {
494         values.push( null ); // no value provided
          }
        }
      }

Line 494 takes something like ?equationsBackgroundColor or ?equationsBackgroundColor= and promotes its value to null.

[pixelzoom]

A couple of test query parameters that I'll be using:

  testString: {
    public: true,
    type: 'string',
    defaultValue: null
  },

  testArray: {
    public: true,
    type: 'array',
    elementSchema: {
      type: 'number'
    },
    defaultValue: null
  }

[pixelzoom]

My main conclusion after trying to move this forward: QueryStringMachine needs a rewrite. What a mess.

[pixelzoom]

Re the promotion of undefined to null noted in https://github.com/phetsims/query-string-machine/issues/43#issuecomment-1258736643 ...

I'm concerned that there's some code that is depending on this. For example:

    const parseFlag = function( key, schema, value ) {
      return value === null ? true : value === undefined ? false : value;
    };

parseFlag makes no sense to me. A flag is not supposed to have a value.

[pixelzoom]

I've spent 3 hours on this issue, and I'm not any closer to addressing any of the problems. Imo, QueryStringMachine needs to be rewritten, it's a mess. So I'm bailing on this, unassigning, and will put it back on the Developer Meeting project board.

Some notes:

• I think it's fine that null is a valid value for 'string' and 'array', and I wouldn't change that.
• I recommend addressing the problem where ?equationsBackgroundColor or ?equationsBackgroundColor= is promoted to null, and then promoted to defaultValue. Those cases should fail, because the URL is malformed (missing the value).

[jbphet]

This issue was discussed in the 11/10/2022 developer meeting, and the group collectively feels like QueryStringMachine has served us well but is in need of some significant rework.

Some thoughts:

• We will need to decide how we want it to support things like creating the needed properties for us, instead of requiring a bunch of work to create and validate them manually now
• Perhaps we should create a "next gen" query string machine and deprecate the old one
• The new one could potentially live in the axon repo
• @kathy-phet said that this might be a good candidate effort for our next sprint

@kathy-phet has put a new placeholder issue on the planning project board, so work on this will be prioritized in one of the upcoming quarterly planning meetings.