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

[zepumph]

This will be helpful for https://github.com/phetsims/ratio-and-proportion/issues/404, and is aligned with quarterly goals.

Note: https://github.com/phetsims/tasks/issues/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 https://github.com/Microsoft/TypeScript/issues/30355#issuecomment-473060190). 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 https://github.com/phetsims/chipper/issues/1127.

[zepumph]

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

[zepumph]

Alrighty. Three days later, I realized that everything was quite interconnected. It was hard to separate the new mixin pattern from the typescript conversion without putting a fair number of bandaids on things. Furthermore, since the mixin hierarchy is pretty much all together, I think it was best to do them all at one. I changed the name of this issue BTW.

A summary of the work completed:

• InteractiveHighlighting, Voicing, ReadingBlock, AccessibleValueHandler, AccessibleSlider, and AccessibleNumberSpinner are all in typescript using the same pattern.
• All usages were converted as well. The main changes for usages were as follows:
  • extending Mixin( Supertype )
  • AccessibleValueHandler is now set up to mutate all options for you, so you must pass in options via super instead of mutate. This involved some finagling
  • Getting rid of initializeX and disposeX (becaus it is part of super now)
  • If the usage is a Typescript file, this pattern means that the subtype isn't known to be a Node, so note many spots where we have X as unknown as Node. This is probably the worst part about the pattern, but perhaps @jonathanolson and I can keep working on it (it's the best we can do right now).
• Please note that there are 96 TODOs pointing to this issue. Most of which duplicate themselves, and revolve around how the anonymous classes that the mixins return can't have private or protected members. There is not a TODO where ever there was a private or protected function, or property that doesn't start with a _. There are also a fair number of these that would be really nice to pair with someone on so that I can feel a bit more confident about wonky changes.

I'll check back in the morning to see what CT has to say on the matter.

And thank you @samreid and @jonathanolson for the help along the way.

[zepumph]

I am not exited about the downstream effects of our mixin pattern at this time. Right now Every Slider is not known as a Node. I'm trying to figure out the best way forward. I tried something like this

const AccessibleSliderClass = AccessibleSlider( Node ); class Slider extends ( AccessibleSliderClass as typeof AccessibleSliderClass & Node ) {

But I got this error on the typecast: "TS2510: Base constructors must all have the same return type."

[zepumph]

I wonder if this pattern could help us https://github.com/microsoft/TypeScript/issues/40110

[jonathanolson]

Right now Every Slider is not known as a Node.

That's confusing, the mix-in pattern presumably should handle that correctly. Can we collaborate again?

[zepumph]

Yes please. I definitely think there will be a workable solution, I just don't have it quite yet.

It also think it is quite likely that it will be complicated enough that when you have a Typescript mixin used by a javascript common code used by a Typescript sim class, the Typescript->Javascript->Typescript will lose the typing specificity, and so to get a working solution, We may need to convert Slider.js and a couple others.

[zepumph]

@jonathanolson showed me a couple of strategies about how to handle mixins

https://github.com/phetsims/wilder/blob/67ba19ab8d5c5ab2302772f568f45721fdb360b2/js/wilder/view/Mixable.ts#L37-L53

We need the same constructor signature everywhere, which means ...args[]:any, and then plucking out our parameters.

[zepumph]

Above is a bandaid. Here are the next steps from another meeting with @jonathanolson. This will provide typesafety, and the flexibility for these mixins to mix into any arbitrary Node class, even when they have extra parameters and or options that must be used in their constructor:

1. only required options, not parameters
2. passing optionArgPlacement into the mixin so we know where the options object is.
3. voicing/interactive mutating their stuff as we go, then pluck it, and always passing everything up

[zepumph]

Lots of progress above. There are still a lot of TODOs in the code, but I think most things are working at this time. The next highest priority is trying to fix regressions that have occurred from having NumberSpinner, NumberPicker, and Slider having options passed to super instead of mutate. For example positioning is broken, see the workaround here:

https://github.com/phetsims/scenery-phet/blob/49d5d37f6973c4d04ac4a707023739cca2c24d3c/js/HeaterCoolerFront.js#L168-L171

I'll keep cracking at things.

[zepumph]

To discuss:

1. Polymorphic constructors and supporting these mixins. Is some sort of assertion possible?
2. Need to mutate transform options after super, in its own mutate call, how to split things out?
3. Convert private members in mixins to _
4. Convention for protected members/methods?
5. @ts-ignores based on the need to have other files in Typescript. (FocusManager, AlertableDef)
6. How to access the global namespace in typescript?
7. Doesn't tandem always need to be the last mutatorKey? @samreid do you remember this at all?
8. Best central place for Utility Typescript Types to live, for example Constructor<>?
9. How to support poolable disposing case https://github.com/phetsims/scenery/blob/1cdbd8e5a4d256c319b4fc71f66aeacc2217e049/js/nodes/RichText.js#L1978
10. How to specify types of subtypes of DOM Events?

I am about to meet with @jonathanolson to solidify a path forward on some of these harder ones.

[zepumph]

Need to mutate transform options after super, in its own mutate call, how to split things out?

• [x] @jonathanolson and I will create Node.REQUIRES_BOUNDS_OPTION_KEYS and only mutate those after there are children.

[zepumph]

Polymorphic constructors and supporting these mixins. Is some sort of assertion possible?

• [x] This is acceptable, but we should document the constraint in our PhET mixin pattern doc.

[zepumph]

Convention for protected members/methods?

• [x] Use @protected jsdoc, that will be good enough

[zepumph]

Best central place for Utility Typescript Types to live, for example Constructor<>?

• [x] Let's just create a Constructor.ts file in phet-core.

[zepumph]

Doesn't tandem always need to be the last mutatorKey? @samreid do you remember this at all?

@samreid nevermind, we figured it out.

Here is the pattern for mutator keys in mixins:

VoicingClass.prototype._mutatorKeys = VOICING_OPTION_KEYS.concat( VoicingClass.prototype._mutatorKeys );
  assert && assert( VoicingClass.prototype._mutatorKeys.length === _.uniq( VoicingClass.prototype._mutatorKeys ).length, 'duplicate mutator keys in Voicing' );

• [x] Commit this for our mixins.

No need to touch Paintable usages or Image. They are working fine.

[zepumph]

Above, all option keys should now be respected, and transform is no longer buggy. I could remove the workaround in HeaterCoolerFront.

[zepumph]

• [x] Create Voicing.initialize and Voicing.clean to support Poolable Node usages (RichTextLeaf)

[zepumph]

Thanks tremendously for all the help that @jonathanolson has given over the past couple of days. Things are in a much more stable place now. All bugs that I know of have been taken care of (save https://github.com/phetsims/molarity/issues/242). Now the rest of the work is just cleanup, mostly about documenting private/protected members of the mixins.

[zepumph]

@jonathanolson, I think that you should be the one that takes a look before removing the blocking label.

[zepumph]

This would block the RC of Molecule Shapes, which I think is right on the horizon.

[zepumph]

I created https://github.com/phetsims/scenery/issues/1348 and now there are only 18 TODOs for this issue. @jonathanolson please glance at them as part of the review to see if any feel blocking to you.

[zepumph]

Above is a bug fix reported in https://github.com/phetsims/gravity-force-lab-basics/issues/319 https://github.com/phetsims/ratio-and-proportion/issues/426 https://github.com/phetsims/molarity/issues/242

Because I wasn't passing the optionized options through in AccessibleNumberSpinner or AccessibleSlider. Basically all start/endDrag listeners weren't firing. Fixed above!

[zepumph]

My recommendation about how to review:

1. Be file-centric instead of commit centric. Much of the decisions on changes were made after discussion with you, so most new patterns will be familiar. Please take a look at all six mixins:
   • InteractiveHighlighting
   • Voicing
   • ReadingBlock
   • AccessibleValueHandler
   • AccessibleSlider
   • AccessibleNumberSpinner
2. Then take a look at usages of these, by searching for Mixin(
3. Then take a look at all the TODOs that still point to this issue, noting that private renaming will be done in https://github.com/phetsims/scenery/issues/1348.
4. Thanks!

[jonathanolson]

In TwoSceneSelectionNode, there is:

    super();

    // Separate call to support mutating InteractiveHighlight options
    this.mutate( options );

Is that... needed anymore? My understanding is that it's not.

InteractiveHighlightInteractionNode in BalloonNode also has that.

[jonathanolson]

Lots of things are mixing into the same base type... memoizing sounds helpful there (but would need to figure out how we should memoize with the 2 parameters). Otherwise we're losing the in-memory benefits.

Perhaps we can programmatically determine the number of supertype arguments and autodetect the optionsArgPosition?

The mixed superclass is separated into a separate once-used variable. Is that on purpose?

[jonathanolson]

@zepumph questions listed above, have a few inline REVEW comments, and overall the changes in the above commits (mostly already discussed). Can you review the changes above and see if those look good?

The structure of everything looks great and as anticipated. I'll be looking into a few ways we could improve the typescript bits, since we don't have much type protection or conciseness in a few cases.

[jonathanolson]

It looks like https://github.com/phetsims/friction/commit/efcc0c2ea90f2edbb7a94c41ec764e725870d2c5 caused a visual regression. It moved super( options ) to super() with a mutate( options ) at the end, changing behavior. We should scan other commits to see if this pattern happened in any sim repo. @zepumph thoughts on whether this was done often?

[jonathanolson]

Why did https://github.com/phetsims/inverse-square-law-common/commit/7dce161a32ac74e2fc943608c09db0db6a8d6ab1 split ISLCForceArrowNode's super( options ) into a super and mutate next to each other?

[jonathanolson]

https://github.com/phetsims/balloons-and-static-electricity/commit/c5de3ea38214f8678492fb694bfd5f4349c7033d has a similar split

[jonathanolson]

https://github.com/phetsims/scenery-phet/commit/d347e7d4b256e98e31f80de106389c89e71d833b and FaucetNode / NumberPicker / SpectrumSlider should be checked

[jonathanolson]

SpectrumSlider has an earlier options (in the super) instead of a mutate at the end, positioning code based on it could be off.

[jonathanolson]

FaucetNode also has earlier options that should be mutated near the end. NumberPicker looks like it has the bounds-related options put at the end and is likely good.

[jonathanolson]

https://github.com/phetsims/sun/commit/1ad9e76414658b3797009662e651d3d41b48d34a NumberSpinner also needs this.

[pixelzoom]

To be on the safe side, I’m going to consider this blocking for https://github.com/phetsims/function-builder/issues/139, and not proceed with publishing Function Builder for dev testing. Function Builder uses NumberPicker.

[zepumph]

The broken common code files are NumberSpinner, FaucetNode, and SpectrumSlider. NumberPicker and Slider are working correctly. @pixelzoom please adjust your blocking status accordingly. Commits coming soon for the broken classes listed above, as well as a fair number of sim-specific usages of AccessibleSlider etc.

I will fix this by applying the same strategy @jonathanolson and I discussed and implemented for Slider, like https://github.com/phetsims/sun/blob/35aee14690a552602c5abfd49d8166838dd3d1b8/js/Slider.js#L140-L142.

[zepumph]

I also found a few places where we weren't providing the index of where to find the options arg, this was helpful in finding those cases: (ReadingBlock|InteractiveHighlighting|Accessible[A-Z]\w+)\( \w+ \)

Commits for that, plus an assertion to make sure we don't do so in the future, and the mutate/options fix discussed above coming soon! I'm just testing thoroughly now.

[zepumph]

Lots of things are mixing into the same base type... memoizing sounds helpful there (but would need to figure out how we should memoize with the 2 parameters). Otherwise we're losing the in-memory benefits.

• [x] Is this as simple as adding memoize around InteractiveHighlighting? I'm happy to try that out, but not sure I would measure the improvement that we are hoping for from this. Would you like me to do that?

[jonathanolson]

Is this as simple as adding memoize around InteractiveHighlighting?

Yes hopefully, since it will ignore the params argument that shouldn't change. It may need some changes to type correctly.

Perhaps collaborating (at a lower priority) would be good for this?

[zepumph]

All comments after https://github.com/phetsims/scenery/issues/1340#issuecomment-1032862162 have had a fix applied above. @jonathanolson, can you spot check the above as a micro commit to ensure we are on the same page. The other parts of the review seem lower stakes.

I tested with fuzzing, and with the following:

• Go to the demo for common code components,
• Make the container it is passed to not a LayoutBox (I used Node)
• Pass left/right/top/bottom or some sort of transform option to it
• Ensure nothing has changed on reload

I also added an assertion for passing in a number for optionsArgPosition, and found a couple places I was missing it.

[zepumph]

Perhaps we can programmatically determine the number of supertype arguments and autodetect the optionsArgPosition?

• [ ] The catch to me is that we would most likely need options to be conventionalized to be the last, required parameter. I don't think it makes sense to make it required, it would change usages of these mixins too much. Can you think of another way?

[zepumph]

The mixed superclass is separated into a separate once-used variable. Is that on purpose?

I refactored them to be inline, at one point it was needed, but I don't think I ever even committed that version.

[zepumph]

Lots of TODOs were removed in https://github.com/phetsims/scenery/issues/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?

https://github.com/phetsims/scenery/blob/e826ddacfe51a904aa58e45f517c5afede3812f9/js/accessibility/voicing/Voicing.ts#L241

• [ ] 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?