Convert Voicing.js and others (6 mixins) to new mixin pattern and typescript

[zepumph]

This will be helpful for phetsims/ratio-and-proportion#404, and is aligned with quarterly goals.

Note: phetsims/tasks#1096

[zepumph]

Tagging @jessegreenberg

[zepumph]

I found that I had to make a couple of things public from private/protected in Voicing because of TS4094 (explained in microsoft/TypeScript#30355 (comment)). I marked a couple of TODOs pointing to the change.

I tried to convert Voicing to typescript so that I could specify that it can only provide Node subtypes to be extended, and I ran into that same issue. Since there are protected/private members on Node, it seems like we are a bit stuck. I am pretty sure that I have to give up here. At least for now.

voicingTypescriptPatch.txt (first you need to rename Voicing.js -> Voicing.ts)

I also wanted to tag @samreid because I was talking to him on slack and he mentioned https://www.typescriptlang.org/docs/handbook/mixins.html#constrained-mixins. I believe that this is how we ran into the issue above where we can't have private members.

[samreid]

@jonathanolson is the expert on TypeScript mixins and the constraints around private/public members and may be able to recommend how to proceed here.

Also, I skimmed the patch and am wondering if it would work for this case to do class VoicingNode extends Node instead?

[zepumph]

I do not think so, the current structure is as such for 6+ mixins:

InteractiveHighlighting
  Voicing
    ReadingBlock
    AccessibleValueHander
      AccessibleSlider
      AccessibleNumberSpinner

That said, it is a recent addition from AccessibleValueHandler to extend Voicing, and if we had to, we could likely move away from it (it's sorta up in the air anyways).

Many usages of Voicing and AccessibleValueHandler are added into the middle of hierarchies, and with typescript I wouldn't know how to say Voicing extends Node, but could somehow be tacked into a hierarch under Path/Rectangle etc.

To see usages of this, I searched for extends [^N](.*\n.*)*initializeVoicing, and found that Voicing is mixed into Node, Rectangle, Path, Vbox, RichTextLeaf and many others in the future.

I reached out to @jonathanolson to see if we could move forward with this pattern.

[zepumph]

In a conversation with @jonathanolson this afternoon, he pointed me towards the pattern used in Paintable, as well as the general issue phetsims/chipper#1127.

[zepumph]

So far so good. I'm going to proceed with some of the TODOs in the issue. (currently 21)

[zepumph]

Lots of TODOs were removed in #1351.

[zepumph]

There are only two more open questions for me in this issue. 4 TODOs (2 topics repeated in two spots each):

• @jonathanolson, can you figure out why this is causing a lint error?

scenery/js/accessibility/voicing/Voicing.ts

Line 241 in e826dda

// TODO: Why is this causing a lint error? AlertableDef is in phet-types. https://github.com/phetsims/scenery/issues/1340

• This is for typescript in general in scenery. I'm wondering how scenery listeners can support more specific subtypes of the window.Event interface. For example, I need access to items that only some events have, like KeyboardEvent. Can you recommend how to proceed here?

https://github.com/phetsims/sun/blob/a18ba9c51046f8885d96b3baa1d4ae037fe8c161/js/accessibility/AccessibleNumberSpinner.ts#L112

[jonathanolson]

can you figure out why this is causing a lint error?

Not immediately sure. I don't think that eslint checks the phet-types file, presumably we'd want that type somewhere it's imported.

This is for typescript in general in scenery. I'm wondering how scenery listeners can support more specific subtypes of the window.Event interface. For example, I need access to items that only some events have, like KeyboardEvent. Can you recommend how to proceed here?

Presumably SceneryEvent would support a type parameter, and we could have subtypes so that it's more type-safe.

[zepumph]

I liked typecasting as a KeyboardEvent much more, enough to remove the TODO. Even if this sticks around for a while, it feels pretty low stakes, and safe. I also converted AlertableDef to typescript, just to add the type (TAlertableDef), which is fine for now. We can't rename it until all usages of AlertableDef.isAlertableDef are in typescript files (unless we delete them). I'm ready to close this issue. Anything else here @jonathanolson?

[jonathanolson]

Looks good to close, thanks!