deepEquals function broken on IE

[zepumph]

While investigating with @chrisklus https://github.com/phetsims/energy-forms-and-changes/issues/320, we discovered that there was a bug in QSM when running in IE. The deepEquals function broke on validValues line of this query param schema:

burners: {
    type: 'number',
    defaultValue: EFACConstants.MAX_NUMBER_OF_INTRO_BURNERS,

    // create an array with integer values 1-N, where N is the max number of burners on the intro screen
    validValues: [ ...Array( EFACConstants.MAX_NUMBER_OF_INTRO_BURNERS ) ].map( ( n, i ) => ++i ),
    public: true
  },

Looking further, it seems like deepEquals just wasn't written to fully account for types other than string,object,null, and undefined.

UPDATE: Sorry I didn't explain why this didn't work on IE. In other browsers Object.keys( 3 ) returns an empty array, but in IE it errors out because a number is not an object.

[zepumph]

I put in a stop gap, but I think I would rather see the function rewritten to be something more robust. @samreid what do you think about this rewrite?

Index: js/QueryStringMachine.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/QueryStringMachine.js    (revision 22841a6b2a06ba964322d8c54e3a59a04bd6ba25)
+++ js/QueryStringMachine.js    (date 1584728322618)
@@ -221,30 +221,19 @@
         if ( typeof a !== typeof b ) {
           return false;
         }
-        if ( typeof a === 'string' || typeof a === 'number' || typeof a === 'boolean' ) {
-          return a === b;
-        }
-        if ( a === null && b === null ) {
-          return true;
-        }
-        if ( a === undefined && b === undefined ) {
-          return true;
-        }
-        if ( a === null && b === undefined ) {
-          return false;
-        }
-        if ( a === undefined && b === null ) {
-          return false;
-        }
-        const aKeys = Object.keys( a );
-        const bKeys = Object.keys( b );
-        if ( aKeys.length !== bKeys.length ) {
-          return false;
-        }
-        else if ( aKeys.length === 0 ) {
-          return a === b;
-        }
-        else {
+        if ( typeof a === 'object' ) {
+          if ( a === null && b === null ) {
+            return true;
+          }
+          const aKeys = Object.keys( a );
+          const bKeys = Object.keys( b );
+          if ( aKeys.length !== bKeys.length ) {
+            return false;
+          }
+          else if ( aKeys.length === 0 ) {
+            return a === b;
+          }
+          else {

             for ( let i = 0; i < aKeys.length; i++ ) {
               if ( aKeys[ i ] !== bKeys[ i ] ) {
@@ -257,7 +246,13 @@
               }
             }
             return true;
+          }
         }
+        else {
+
+          // comparing all non-object types
+          return a === b;
+        }
       },

       /**

My thoughts here are that anything that isn't an object should be triple equal checked. Even functions should be supported by deepEquals in my opinion. Next note that undefined is its own type, and doesn't need so much logic added.

I see many commits in QSM by CM and SR, so I randomly selected @samreid to take a look here. What do you think of the patch and my stop gap commit above?

[chrisklus]

What about using _.isEqual instead? I tried it out in the current deepEquals function and the QSM tests all passed.

[zepumph]

QSM was developed to be a standalone, dependency-less library. PhET developed it for use in phet sims, but presumably it could be used anywhere else. With that in mind I think that there should be a very good reason to add lodash as a dependency for it.

Another side note about the original issue: QSM unit tests have always been passing on Chrome because this wasn't a bug there. Currently we don't support tests on IE, and as a result I wasn't able to make a test the exposed this issue as part of my fix above. I'm not sure it is worth the investment right now to develop a way to build and transpile our tests into IE runnable code, but this would have been caught ages ago if qunit was being run on IE.

[chrisklus]

Oh right, I forgot, thanks. @pixelzoom and I did this once already so I opened an issue to remove it, see https://github.com/phetsims/query-string-machine/issues/40.

[samreid]

We will be dropping support for IE in https://github.com/phetsims/tasks/issues/1037, deprioritizing for now.

[samreid]

Now that we have dropped support for IE, and are not planning on testing QSM in IE, how about documenting as "IE not supported" in the QSM README, and closing this issue? @zepumph does that seem like a good plan?

[zepumph]

I thought about writing that in the QSM lib, but isn't it more robust to just recognize that all phet repos have the same browser support as do phetsims in general? Does PhET advertise that in a general place? Perhaps that is a topic for https://github.com/phetsims/tasks/issues/1018?

[samreid]

Even if we document that in a separate place, it is probably worth a separate mention in QSM which is designed to be used independently from the rest of the PhET ecosystem.

For end users, we should document this in https://phet.colorado.edu/en/help-center/running-sims and for developers, how about the phet-development-overview.md?

[zepumph]

Can we handle this issue over in https://github.com/phetsims/tasks/issues/1042 and close this one?

[samreid]

Yes, thanks!