search

phetsims / utterance-queue

Alerting library powered by aria-live
MIT License
0 stars 2 forks source link

Utterance needs a dispose #80

Closed jessegreenberg closed 2 years ago

jessegreenberg commented 2 years ago

Discovered during https://github.com/phetsims/phet-io/issues/1858. Utterance generally needs dispose. We should also look through existing usages of Utterance to make sure that there aren't any memory leaks, particularly in common code usages.

jessegreenberg commented 2 years ago

I think dispose has been implemented by disposing of Properties and clearing the canAnnounceProperties. @zepumph would you like to review this issue? Otherwise this can be collapsed into https://github.com/phetsims/phet-io/issues/1858.

jessegreenberg commented 2 years ago

Wait, we also need to review Utterances in the project and look for leaks.

jessegreenberg commented 2 years ago

I scanned through the project looking for new Utterance (78 hits) as well as things that extend Utterance and its usages. Pretty much all cases keep a reference to the utterance forever in the constructor or as an instance variable and the Utterance never changes. It seems safe to me.

zepumph commented 2 years ago

We do not own canAnnounceProperties, so I don't think we can dispose them.

jessegreenberg commented 2 years ago

That makes sense to me, I don't think we are currently. It looks like we are just clearing the reference to them in our private array https://github.com/phetsims/utterance-queue/blob/2f73d20ba7f8bbdfffb3258695f3e7e37ff8a4a8/js/Utterance.ts#L268-L269

zepumph commented 2 years ago

Ahhh, excellent. Sorry for the trouble. All looks great here. Please reopen if you have more to discuss.