Review TypeScript conversion

[jbphet]

In the 4/14/2022 developer meeting we decided that each developer should do some reviewing of another developer's TypeScript code conversions in a common code library. I was assigned to review this one (vegas).

[jbphet]

For the most part, this all looks great. I added and changed some comments for consistency and handled a ts-nocheck. I also added a handful of REVEW-TS comments, but they are all basically about the same thing, which is whether the score display constructors could be typed in some way so that the comments that say "All constructors must have the same signature!" would be unnecessary and automatically enforced. Maybe static factory methods?

There are a number of usages of any in the code that don't have TODOs associated with them, and they all seem to relate to the pattern, mentioned above, where different constructors are being passed around. I'll also ask about this in the next developer meeting. I imagine that @pixelzoom wouldn't have done these like this if there was an obvious type-specific alternative.

That's it. @pixelzoom - have a look at what I've done and close if you're cool with it. I also have a couple of questions that arose out of this that I'll ask at the dev meeting, but I don't think they're worth putting into this issue. If it turns out that the rest of the team disagrees, I'll amend.

[pixelzoom]

Thanks for the review @jbphet.

There are a number of usages of any in the code that don't have TODOs associated with them,

They are related to #102. I've added TODOs. They are also related to the "All constructors must have the same signature!" issue, so I've replaced your "REVIEW-TS" comments with TODOs.

I also have a couple of questions that arose out of this that I'll ask at the dev meeting, but I don't think they're worth putting into this issue. If it turns out that the rest of the team disagrees, I'll amend.

@jbphet what were your general questions? I might be able to answer them, before bringing to a larger forum.

[jbphet]

Here are the items that I put in the dev meeting notes. These are mostly for my own edification, but since you asked:

• Is there a way that constructors with the same signature can be typed (I’ll show an example of why this is necessary)? What about using a static factory method instead?
• There are some usages of any related to the constructors. Is there a better way?
• There are a number of usages of merge that don’t seem entirely typesafe. Are these all cool? My goal is to get to where I know when to use merge versus optionize.

[pixelzoom]

• Is there a way that constructors with the same signature can be typed (I’ll show an example of why this is necessary)? What about using a static factory method instead?

Yes. See TButtonAppearanceStrategy and TContentAppearanceStrategy. Something similar could be used for the "score display" classes in vegas. Perhaps using an interface is another alternative, though I'm not sure if an interface lets you specify the constructor signature.

• There are some usages of any related to the constructors. Is there a better way?

If you're referring to:

  // score display
  scoreDisplayConstructor?: any; //TODO https://github.com/phetsims/vegas/issues/102
  scoreDisplayOptions?: any; //TODO https://github.com/phetsims/vegas/issues/102

I think we'll end up converting those to functions that return an instance, something like:

   createScoreDisplay: ( scoreProperty: IProperty<number> ) => TScoreDisplay

• There are a number of usages of merge that don’t seem entirely typesafe. Are these all cool? My goal is to get to where I know when to use merge versus optionize.

That question has been posed in phetsims/chipper#1226.

[jbphet]

Thanks for the insights. I'm okay to close this and move on if you are.

[pixelzoom]

👍🏻

[pixelzoom]

Btw... There were a few uses of merge that I converted to optionize in the above commit. The remaining uses of merge are related to #102 and now have an associated TODO.