Convert to typescript and resolve API type errors

zepumph commented 2 years ago

This means a couple of things:

[x] resolve the difference between the type that Voicing supports, and Description, and how that works for UtteranceQueue.
[x] Remove the support for looping alerts in Utterance.
[x] Convert files to typescript, and make sure that all typing exists harmoniously.
[x] Get rid of AlertableDef.
[x] make cancelUtterance and announce abstract after converting SpeechSynthesisUtterance to typescript

Note https://github.com/phetsims/tasks/issues/1096

zepumph commented 2 years ago

The main thought @jessegreenberg and I had this afternoon was to have Utterances support functions. And update Utterance so that it is easier to get the "alert text" from an Utterance to pass to voicing .This will better align the two APIs. Mainly this work was brought about by https://github.com/phetsims/sun/issues/742.

zepumph commented 2 years ago

[x] One thing to discuss with this is that I made null an acceptable member of IAlertable. I want to talk about how to handle that case. Perhaps if the Utterance has null alert, then we just ignore it. @jessegreenberg what do you think?

jessegreenberg commented 2 years ago

Was reviewing this issue and noticed that we lost history for Utterance, Ill see if I can restore it. Other TS files in utterance-queue have history still.

jessegreenberg commented 2 years ago

Here is how I ended up doing it. Im sure there is a cleaner way with git without having to manually revert, but some other things seemed too risky or had the same result of not saving history.

Manually revert all commits (7) until the one before the TypeScript converson.
git cherry-pick -n edaef5b00f3308912faf3f2f9abcf4e30ca22462 (the TS conversion SHA)
git restore --staged . to unstage everything
git add everything except Utterance.ts and Utterance.js
Delete Utterance.ts and restore Utterance.js
Commit this (with --no-verify)
Rename Utterance.js -> Utterance.ts and commit (with --no-verify)
Manually copy change from https://github.com/phetsims/utterance-queue/commit/edaef5b00f3308912faf3f2f9abcf4e30ca22462 into Utterance.ts.
Cherry-pick the commits after the TypeScript conversion back onto master.

After all of this I verified that we still have history for all .ts files in utterance-queue.

jessegreenberg commented 2 years ago

One thing to discuss with this is that I made null an acceptable member of IAlertable.

I am not sure I understand but this seems fine to me. null was the default value for alert before this change so hopefully it should be supported. null is a valid value of ResolvedResponse so do we also need it in the type for _alert?

  private _alert: AlertableNoUtterance | null;

zepumph commented 2 years ago

Oh wow. Thank you so much! I really appreciate you picking up the git history slack. I should have know better.

I removed duplicate null types, and generally cleaned it up. Thank you!

I'm not yet confident that UtteranceQueue is totally set up to our new Utterance.alert type, I want to take another check on this. And then perhaps convert a bit more to typescript and check the rest of the boxes above.

zepumph commented 2 years ago

OK. I believe that I'm done with this issue. @jessegreenberg, everything in this batch of commits here was a straight forward refactor except one item. I found that it felt sketchy to have a getSynth() that could return null on a platform that doesn't support speech synthesis. We have some support for that case, but I think we rely on assertions a lot to guard against null pointer exceptions. In the wild, we wouldn't have assertions so we may want to be graceful. I added a couple of spots that checked if we had a synth before calling operations on it. Does this seem correct to you? I'd hate for the sim to break just because the platform doesn't have speech synthesis. For example, in the app, right? Should we make a separate issue for this?

Here is an example:

https://github.com/phetsims/utterance-queue/blob/38f0f331338b61caed1ef8cea57e03741c8a24b9/js/SpeechSynthesisAnnouncer.ts#L340-L348

Can you please spot check the conversion. Thanks!

zepumph commented 2 years ago

Oops sorry, I forgot to assign you.

jessegreenberg commented 2 years ago

I took a look at the TS conversion and things look good. I mostly reviewed the public API like options. I also verified that we still have history for everything.

I found that it felt sketchy to have a getSynth() that could return null on a platform that doesn't support speech synthesis.

I have thoughts, but lets create a new issue.

Back to you to close if it is time.

phetsims / utterance-queue

Convert to typescript and resolve API type errors #67