Type casting SpeechSynthesisAnnouncer.enabledProperty is potentially buggy

[chrisklus]

In the TypeScript channel, I asked about a type error I'm getting in WebStorm that is not showing up in pre-commit hooks:

Chris Klusendorf 7:15 PM has anyone run into a problem where Webstorm errors on .value = for an IProperty, but tsc is passing? @Luisa and I have experienced this in separate cases today Chris Malley 7:20 PM voicingManager.ts is a singleton of VoicingManager, which extends SpeechSynthesisAnnouncer. And there’s some hackery in SpeechSynthesisAnnouncer: 230 this.enabledProperty = this.enabledComponentImplementation.enabledProperty as IProperty; I wouldn’t be surprised if that is confusing WebStorm. 7:21 And indeed, this.enabledComponentImplementation.enabledProperty is actually defined like this in EnabledComponent: public enabledProperty: IReadOnlyProperty; So it is in fact readonly. And casting it to IProperty is super buggy. 7:23 The cast was added in https://github.com/phetsims/utterance-queue/commit/d4149e203425e13870c04d5c927d03512577d9fa. I suggest a GitHub issue, assigned to the caster.

@samreid could you please take a look at your convienience?

[jessegreenberg]

I don't really understand how EnabledComponent is being used here, ill try to take a look too.

[jessegreenberg]

It may be that SpeechSynthesisAnnouncer is not a good use case for EnabledComponent. It cannot extend EnabledComponent and opts out of phet-io. It could be simpler to just use our own Property<boolean>.

Alternatively, does EnabledComponent need to support taking read-only Properties? Could the type of enabledProperty change to a BooleanProperty (the supertype of its default EnabledProperty)? Then clients can do what they wish with the Property and the type cast in EnabledComponent.setEnabled would not be necessary.

@zepumph may have thoughts about this too since I think he added the EnabledComponent portion to SpeechSynthesisAnnouncer.

[zepumph]

I looks like this was a bug introduced in https://github.com/phetsims/axon/commit/a2730117d747a1f746a73941c66ad04b1e12cab8.

It looks like we need to make EnabledComponent parameteric to whether or not you can set the enabledProperty. I'll take a look.

[zepumph]

@samreid, @pixelzoom, @zepumph had a talk about public APIs for subcomponent properties (visibleProperty/enabledProperty/etc). Generally typescript behaves poorly here, because we have to parametrize the class to say if the underlying enabledProperty is settable or not. This would involve passing that parameter up from EnabledComponent -> ButtonModel -> forever up the hierarchy. Not to mention the need to do this for each subProperty of Node. Imagine: Node<VisiblePropertyType, EnabledPropertyType, PickablePropertyType, InputEnabledPropertyType>.

So we instead decided to punt. EnabledComponent accepts an IReadonlyProperty as an option, and this.enabledProperty is an IProperty. We type cast it internally. This is pretty excellent from a public API perspective, but it relies on runtime assertions when you try to set a derived property.

@chrisklus I didn't see your issue in NumberPlay anymore, and was able to remove the type cast in SpeechSynthesisAnnouncer.

Please review.

[chrisklus]

@zepumph changes look great, and thanks for the explanation! I have not seen this issue on my end anymore. Closing.