TypeScript review

[chrisklus]

TypeScript review of utterance-queue for developer review rotation.

I reviewed all of the files in TypeScript and they are looking great - TS conventions are being followed very well. I wrote out a few small things to consider/things to fix:

• In AriaLiveAnnouncer.ts, it seems a little weird to have two types that both contain 'polite' and 'assertive' (AriaLivePriorityString and AriaLive). One comment for AriaLive reads Possible values for the aria-live attribute (priority) that can be alerted (like "polite" and // "assertive"), but it seems like AriaLive is an Enumeration and AriaLivePriorityString contains the actual values. Perhaps it would be simpler and clearer to just have the type AriaLive be a rich enumeration by having the string values be added to each enum value as something like string.
• In responseCollector.ts, ResponsePacket.ts, and ResponsePatternCollection.ts is there a reason optionize3 is being used since the first object is empty?
• Optionize pattern in SpeechSynthesisAnnouncer.ts:
  • SpeechSynthesisAnnouncerOptions and AnnouncerOptions are currently the same, but it looks like they are swapped in position in the optionize call so there will be problems if any SelfOptions are added to to SpeechSynthesisAnnouncerOptions.
  • From a very recent Slack discussion, it was recommended to still include a SelfOptions type, even when there are currently no SelfOptions, see (2) in https://github.com/phetsims/phet-info/blob/master/doc/phet-software-design-patterns.md#options-typescript. SpeechSynthesisAnnouncer.ts has EmptyObjectType in its optionize call instead of SelfOptions, so could be an place to update if desired. Also noticed this in ValueChangeUtterance.ts

• In SpeechSynthesisAnnouncer.ts, a comment noting why the synth will be available at these points might be helpful. Most other places use an if check. Although, both functions have an SpeechSynthesisAnnouncer.isSpeechSynthesisSupported() assertion, is that checking the same thing (and therefore enough)?

  // browsers tend to generate the list of voices lazily, so the list of voices may be empty until speech is
  // first requested
  this.getSynth()!.onvoiceschanged = () => {
    this.populateVoices();
  };

  this.getSynth()!.speak( speechSynthUtterance );

• In UtteranceQueue.ts, the optionize call is using PhetioObject instead of PhetioObjectOptions for the parent options:

  const options = optionize<UtteranceQueueOptions, SelfOptions, PhetioObject>()( {

Assigning to the responsible dev @jessegreenberg for next steps, nice work!

[jessegreenberg]

Great, thanks! I believe the pairing for this review exercise was intended to be @zepumph so assigning to him but let me know if there is anything to be discussed in the recommendations.

[chrisklus]

Thanks for the clarification! I thought so too but then saw you're the responsible dev so I thought maybe there was a mixup, oops.

[zepumph]

• In AriaLiveAnnouncer.ts, it seems a little weird to have two types that both contain 'polite' and 'assertive' (AriaLivePriorityString and AriaLive). One comment for AriaLive reads Possible values for the aria-live attribute (priority) that can be alerted (like "polite" and // "assertive"), but it seems like AriaLive is an Enumeration and AriaLivePriorityString contains the actual values. Perhaps it would be simpler and clearer to just have the type AriaLive be a rich enumeration by having the string values be added to each enum value as something like string.

In general I feel like the RichEnumeration (class based) strategy like AriaLive is nice, comfortable and clear, especially because announcerOptions typing is still a bit weird since any Announcer can declare their own, but the interface is set on Utterance. In createBatchOfPriorityLiveElements we are actually setting the aria-live attribute, which needs to be the actual strings "polite" or "assertive". I liked noting that with typescript rigidity in the private type AriaLivePriorityString. I see no other way of ensuring that we don't accidentally set an aria-live attribute to an unsupported value. I'm open to ideas though!

---

• In responseCollector.ts, ResponsePacket.ts, and ResponsePatternCollection.ts is there a reason optionize3 is being used since the first object is empty?

Yes that is exactly right, optionize3 is only for using optionize in the case where you can't merge into the defaults, see the typing here where the first argument should be an empty object:

https://github.com/phetsims/phet-core/blob/a9baa22cce8a524bfc28bdf0afda4ea367c382d6/js/optionize.ts#L78

I updated some documentation

---

Optionize pattern in SpeechSynthesisAnnouncer.ts:

Agreed. The first was a clear bug, and the second is an improvement. Done.

---

• In SpeechSynthesisAnnouncer.ts, a comment noting why the synth will be available at these points might be helpful. Most other places use an if check. Although, both functions have an SpeechSynthesisAnnouncer.isSpeechSynthesisSupported() assertion, is that checking the same thing (and therefore enough)?

There is an assertion at the top of requestSpeech, which I think is the best place for an eager assertion, and I feel totally fine about calling getSynth in that function. In the constructor, we have set this.synth just 10 lines above.

Would you like documentation in those spaces like "already asserted above" or "just set this.synth" in those cases? I think it is a bit unnecessary but I'm happy to if you'd prefer (even slight preference).

---

In UtteranceQueue.ts, the optionize call is using PhetioObject instead of PhetioObjectOptions for the parent options:

Ooops.

[zepumph]

@chrisklus and I discussed the above. and https://github.com/phetsims/phet-core/commit/c390f0aa2c786c51d503df64cc335845394b82b4 seems like an improvement. Thanks for the review!