ValueType does not support abstract classes.

[pixelzoom]

I ran into this during equality-explorer https://github.com/issues?utf8=✓&q=is%3Aopen+is%3Aissue+assignee%3Apixelzoom (convert to TypeScript), in the context of Emitter.

In Validation.ts, ValueType does not support abstract classes:

type ValueType = string | Constructor | EnumerationDeprecated | null | ValueType[];

And the problem seems to be in the definition of Constructor. From Constructor.ts:

type Constructor<T=object, K extends IntentionalAny[] = IntentionalAny[]> = new ( ...args: K ) => T;

Here is a minimal example:

class X {}

abstract class Y {
  protected constructor() {}
}

class MyClass {
  public readonly myEmitter: Emitter<[ X, Y ]>;
  public constructor() {
    this.myEmitter = new Emitter( {
      parameters: [
        { valueType: X },
        // TS2322: Type 'typeof Y' is not assignable to type 'ValueType | ValueType[] | undefined'.
        //  Type 'typeof Y' is not assignable to type 'Constructor<object, any[]>'.
        //   Cannot assign an abstract constructor type to a non-abstract constructor type.
        { valueType: Y }
      ]
    } );
  }
}

This Stackoverflow thread notes that abstract could be added to the definition of Constructor.ts:

- type Constructor<T=object, K extends IntentionalAny[] = IntentionalAny[]> = new ( ...args: K ) => T;
+ type Constructor<T=object, K extends IntentionalAny[] = IntentionalAny[]> = abstract new ( ...args: K ) => T;

But then the problem becomes that abstract constructors are typically protected, and that's flagged as a TS error:

TS2322: Type 'typeof Y' is not assignable to type 'ValueType | ValueType[] | undefined'.   Type 'typeof Y' is not assignable to type 'Constructor<object, any[]>'.     Cannot assign a 'protected' constructor type to a 'public' constructor type.

Changing an abstract constructor to public makes WebStorm complain (rightly) with:

Abstract class constructor can be made protected

The workaround in equality-explorer was to use isValidValue instead of valueType. For example, in TermCreator.js, where TermCreator and Term are both abstract classes:

  public readonly termCreatedEmitter: Emitter<[ TermCreator, Term, PressListenerEvent | null ]>;
...
    this.termCreatedEmitter = new Emitter( {
      parameters: [
-       { valueType: TermCreator  },
+      { isValidValue: value => value instanceof TermCreator  }, // cannot use valueType because Term is abstract
-       { valueType: Term  },
+       { isValidValue: value => value instanceof Term  }, // cannot use valueType because Term is abstract
        { valueType: [ SceneryEvent, null ] }
      ]
    } );

[pixelzoom]

In the context of ValueType, we're interested in being able to determine the type of a value, not in being able to call new on the constructor that describes the type of that value. So while Constructor is likely useful and correct in other places that it's used, it does not seem like a good fit for ValueType. So I don't think that the solution here is to modify Constructor.ts. The solution is likely to replace Constructor with something more appropriate for ValueType.

[pixelzoom]

equality-explorer workarounds are shown in the above commit, https://github.com/phetsims/equality-explorer/commit/bda0a1514a4e34f3f15cb6b11c7b1c05dbeeaa40.

[samreid]

I wrote this code to ask TypeScript what type an instanceof check must be against:

const x = 7;
x instanceof 7;

TypeScript replied:

TS2359: The right-hand side of an 'instanceof' expression must be of type 'any' or of a type assignable to the 'Function' interface type

Therefore, it seems appropriate to replace Constructor with Function in ValueType. I'll do so momentarily.

[samreid]

I updated ValueType and updated the occurrences that were using the isValidValue workaround, including in equality-explorer. @pixelzoom can you please review?

[pixelzoom]

I can now use valueTypewith abstrat classes in equality-explorer. But since I'm not very familiar with with Validation.ts or this validation in general, I think that @zepumph should take a quick look.

[zepumph]

I am pretty sure that https://github.com/phetsims/phet-io/commit/546046240fe6f47ce5c122da8b1297239cafb698 will cause problems, as that global may not be available at load time. That is why I remember making a closure around it. Otherwise this all makes sense to me.

[samreid]

Great point! I reverted that and didn't see any other occurrences. Closing, but @zepumph or @pixelzoom please reopen if there's more to do.