search

phetsims / axon

Axon provides powerful and concise models for interactive simulations, based on observable Properties and related patterns.
MIT License
10 stars 8 forks source link

Can we get rid of getListenerCount? #403

Closed samreid closed 1 year ago

samreid commented 1 year ago

From https://github.com/phetsims/axon/issues/402, @marlitas and I would like to remove getListenerCount from the Emitter and Property interfaces. Current usages seem to only be in tests. Can we get rid of the tests? If not, perhaps subclass and make that method public?

samreid commented 1 year ago

@zepumph said maybe use the classes instead of the IEmitter type here. He also said we could ts-ignore usages of private members at the test site.

zepumph commented 1 year ago

Ideally a test file should be tied to a class anyways, so using that specific class seems best. That said I have no nice ideas about how to keep this item on the class private when using it in tests, but I run into this all the time and I hate marking these fields as public.

zepumph commented 1 year ago

How about this? It doesn't solve the general sentiment about wanting to use private items in tests. I would almost prefer hacking the types in tests, likely with ts-ignore statements, but that is likely not a universal preference across the team.

samreid commented 1 year ago

I removed hasListeners from the Emitter type alias. Thankfully getListenerCount was already missing from the type alias. I think it's OK that the concrete Emitter class has these extra methods, but it's nice to have them out of the type alias. I also observed that ReadOnlyProperty has:

  private getListenerCount(): number {
    return this.tinyProperty.getListenerCount();
  }

which is called many times in unit tests--but that's working because the unit tests are in .js instead of .ts. Once moved to *.ts we may need to ts-ignore them or delete these tests.

I'm ready to close this issue, but anyone feel free to reopen if there's more to do here.