Emitter deserves an interface

[zepumph]

@samreid and I discussed this today, and thought it was a bit weird to not have Emitter and TinyEmitter implementing an interface. This will at the very least keep their APIs in sync (they are almost identical currently but not quite, and we should work that out).

This is also helpful for the cases where Emitters are dependency injected, for example:

https://github.com/phetsims/phet-io/blob/917a4a49656af32eac764b715e84de3859f27717/js/phetioEngine.ts#L269

PhetioEngine does not need to know about the guts of Emitter, it should use IEmitter, just like how we use IReadOnlyProperty.

We also thought that in general we should use IEmitter as typescript

We want to use an explicit IEmitter instead of using TinyEmitter's type dynamically in Emitter, not like this:

export default class Emitter<T extends EmitterParameter[] = []> extends PhetioDataHandler<T> implements TinyEmitter<T> {

[pixelzoom]

In light of https://github.com/phetsims/chipper/issues/1283, perhaps a type should be used instead of interface.

[samreid]

Working progress:

```diff Index: main/axon/js/createObservableArray.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/axon/js/createObservableArray.ts b/main/axon/js/createObservableArray.ts --- a/main/axon/js/createObservableArray.ts (revision 7e61dcbd52e75bf028c9dbfc6d5679a74bd15583) +++ b/main/axon/js/createObservableArray.ts (date 1658259158006) @@ -24,6 +24,7 @@ import NumberProperty from './NumberProperty.js'; import Validation from './Validation.js'; import EmptyObjectType from '../../phet-core/js/types/EmptyObjectType.js'; +import IEmitter from './IEmitter.js'; // NOTE: Is this up-to-date and correct? Looks like we tack on phet-io stuff depending on the phetioType. type ObservableArrayListener = ( element: T ) => void; @@ -60,8 +61,8 @@ applyState: ( state: ObservableArrayStateObject ) => void; // listen only please - elementAddedEmitter: Emitter<[ T ]>; - elementRemovedEmitter: Emitter<[ T ]>; + elementAddedEmitter: IEmitter<[ T ]>; + elementRemovedEmitter: IEmitter<[ T ]>; lengthProperty: NumberProperty; //TODO https://github.com/phetsims/axon/issues/334 Move to "prototype" above or drop support Index: main/axon/js/IEmitter.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/axon/js/IEmitter.ts b/main/axon/js/IEmitter.ts new file mode 100644 --- /dev/null (date 1658259816712) +++ b/main/axon/js/IEmitter.ts (date 1658259816712) @@ -0,0 +1,24 @@ +// Copyright 2022, University of Colorado Boulder + +/** + * A simple Emitter/TinyEmitter like interface + * + * @author Sam Reid (PhET Interactive Simulations) + */ + +import IntentionalAny from '../../phet-core/js/types/IntentionalAny.js'; + +// undefined and never are not allowed as parameters to Emitter +export type EmitterParameter = Exclude; + +export type EmitterListener = ( ...args: T ) => void; + +type IEmitter = { + emit: ( ...args: T ) => void; + addListener: ( listener: EmitterListener ) => void; + removeListener: ( listener: EmitterListener ) => void; + dispose: () => void; +}; + +export default IEmitter; + Index: main/bamboo/js/ChartTransform.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/bamboo/js/ChartTransform.ts b/main/bamboo/js/ChartTransform.ts --- a/main/bamboo/js/ChartTransform.ts (revision 9a7a2d3ff1826974c30d622e571efc8e61a40ef3) +++ b/main/bamboo/js/ChartTransform.ts (date 1658259881845) @@ -8,6 +8,7 @@ */ import Emitter from '../../axon/js/Emitter.js'; +import IEmitter from '../../axon/js/IEmitter.js'; import Range from '../../dot/js/Range.js'; import Transform1 from '../../dot/js/Transform1.js'; import Utils from '../../dot/js/Utils.js'; @@ -34,7 +35,7 @@ class ChartTransform { // fires when some aspect of this transform changes - public readonly changedEmitter: Emitter<[]>; + public readonly changedEmitter: IEmitter<[]>; public viewWidth: number; public viewHeight: number; Index: main/axon/js/Emitter.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/axon/js/Emitter.ts b/main/axon/js/Emitter.ts --- a/main/axon/js/Emitter.ts (revision 7e61dcbd52e75bf028c9dbfc6d5679a74bd15583) +++ b/main/axon/js/Emitter.ts (date 1658259816738) @@ -12,7 +12,6 @@ import optionize from '../../phet-core/js/optionize.js'; import EmptyObjectType from '../../phet-core/js/types/EmptyObjectType.js'; import StrictOmit from '../../phet-core/js/types/StrictOmit.js'; -import IntentionalAny from '../../phet-core/js/types/IntentionalAny.js'; import FunctionIO from '../../tandem/js/types/FunctionIO.js'; import IOType from '../../tandem/js/types/IOType.js'; import VoidIO from '../../tandem/js/types/VoidIO.js'; @@ -20,16 +19,11 @@ import axon from './axon.js'; import TinyEmitter from './TinyEmitter.js'; import Tandem from '../../tandem/js/Tandem.js'; +import { EmitterListener, EmitterParameter } from './IEmitter.js'; -// By default, Emitters are not stateful // By default, Emitters are not stateful const PHET_IO_STATE_DEFAULT = false; -// undefined and never are not allowed as parameters to Emitter -type EmitterParameter = Exclude; - -type Listener = ( ...args: T ) => void; - type SelfOptions = EmptyObjectType; type EmitterOptions = SelfOptions & StrictOmit; @@ -80,14 +74,14 @@ /** * Adds a listener which will be called during emit. */ - public addListener( listener: Listener ): void { + public addListener( listener: EmitterListener ): void { this.tinyEmitter.addListener( listener ); } /** * Removes a listener */ - public removeListener( listener: Listener ): void { + public removeListener( listener: EmitterListener ): void { this.tinyEmitter.removeListener( listener ); } @@ -101,7 +95,7 @@ /** * Checks whether a listener is registered with this Emitter */ - public hasListener( listener: Listener ): boolean { + public hasListener( listener: EmitterListener ): boolean { return this.tinyEmitter.hasListener( listener ); } @@ -124,7 +118,7 @@ * @param name - debug name to be printed on the console * @returns - the handle to the listener added in case it needs to be removed later */ - public debug( name: string ): Listener { + public debug( name: string ): EmitterListener { const listener = ( ...args: T ) => console.log( name, ...args ); this.addListener( listener ); return listener; Index: main/bamboo/js/demo/DemoChartCanvasNode.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/bamboo/js/demo/DemoChartCanvasNode.ts b/main/bamboo/js/demo/DemoChartCanvasNode.ts --- a/main/bamboo/js/demo/DemoChartCanvasNode.ts (revision 9a7a2d3ff1826974c30d622e571efc8e61a40ef3) +++ b/main/bamboo/js/demo/DemoChartCanvasNode.ts (date 1658259881853) @@ -25,12 +25,12 @@ import TickLabelSet from '../TickLabelSet.js'; import TickMarkSet from '../TickMarkSet.js'; import CanvasGridLineSet from '../CanvasGridLineSet.js'; -import Emitter from '../../../axon/js/Emitter.js'; import CanvasPainter from '../CanvasPainter.js'; +import IEmitter from '../../../axon/js/IEmitter.js'; class DemoChartCanvasNode extends Node { - public constructor( emitter: Emitter<[ number ]>, options?: NodeOptions ) { + public constructor( emitter: IEmitter<[ number ]>, options?: NodeOptions ) { super(); Index: main/axon/js/TinyEmitter.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/axon/js/TinyEmitter.ts b/main/axon/js/TinyEmitter.ts --- a/main/axon/js/TinyEmitter.ts (revision 7e61dcbd52e75bf028c9dbfc6d5679a74bd15583) +++ b/main/axon/js/TinyEmitter.ts (date 1658260040485) @@ -9,18 +9,17 @@ import IntentionalAny from '../../phet-core/js/types/IntentionalAny.js'; import axon from './axon.js'; +import IEmitter, { EmitterListener, EmitterParameter } from './IEmitter.js'; // constants const shuffleListeners = _.hasIn( window, 'phet.chipper.queryParameters' ) && phet.chipper.queryParameters.shuffleListeners; -type Listener = ( ...args: T ) => void; - type EmitContext = { index: number; - listenerArray?: Listener[]; + listenerArray?: EmitterListener[]; }; -export default class TinyEmitter { +export default class TinyEmitter implements IEmitter { // Not defined usually because of memory usage. If defined, this will be called when the listener count changes, // e.g. changeCount( {number} listenersAddedQuantity ), with the number being negative for listeners removed. @@ -30,15 +29,15 @@ public isDisposed?: boolean; // If specified, this will be called before listeners are notified. - private readonly onBeforeNotify?: Listener | null; + private readonly onBeforeNotify?: EmitterListener | null; // The listeners that will be called on emit - private listeners: Set>; + private listeners: Set>; // During emit() keep track of iteration progress and guard listeners if mutated during emit() private emitContexts: EmitContext[]; - public constructor( onBeforeNotify?: Listener | null ) { + public constructor( onBeforeNotify?: EmitterListener | null ) { if ( onBeforeNotify ) { this.onBeforeNotify = onBeforeNotify; ```

[samreid]

@marlitas and I added IEmitter in the commit and converted declarations of TinyEmitter to IEmitter. But despite yesterday's conversation about choosing the appropriate return/declare type, we thought in some cases it might be appropriate to look at declarations so the developer can instantly see it is the "low memory" variant even though details from that interface are not used. Anyways, we weren't sure about that but wanted to keep going.

[samreid]

This has been implemented and seems to be working well. Some notes:

• We left isDisposed out of the interface since it differs dramatically between Emitter and TinyEmitter. TinyEmitter only allocates space for that when assertions are enabled.
• We opened #403 to ask about getting rid of getListenerCount, which is only used in tests.

Outstanding topics for discussion:

• Should it be called IEmitter? https://github.com/phetsims/chipper/issues/1287
• Should we sometimes prefer TinyEmitter to IEmitter declarations in scenery so it is obvious that it is using the low-memory variant? Even though we don’t use details from that interface.

Otherwise, this issue is ready for review.

[zepumph]

We left isDisposed out of the interface since it differs dramatically between Emitter and TinyEmitter. TinyEmitter only allocates space for that when assertions are enabled.

Yes. That is totally right in my opinion.

Should it be called IEmitter?

https://github.com/phetsims/chipper/issues/1287 is no longer blocking. We want to name the interface Emitter and name Emitter.ts to something else. What should that be?

Should we sometimes prefer TinyEmitter to IEmitter declarations in scenery so it is obvious that it is using the low-memory variant? Even though we don’t use details from that interface.

We discussed this at a dev meeting. The public declaration should be an IEmitter if that is all users of the type need to know. The implementation can still be TinyEmitter.

[samreid]

@marlitas and I reviewed TinyEmitter, IEmitter, Emitter and several usage sites and we recommend the following steps:

• [ ] Rename Emitter => PhetioEmitter. This makes sense because PhET-iO is the biggest difference compared to TinyEmitter (in terms of behavior, code complexity, memory footprint, etc.). PhetioEmitter extends PhetioDataHandler and in turn extends PhetioObject. So PhetioEmitter is a natural name for this.
• [ ] Rename IEmitter => Emitter as agreed above
• [ ] Visit usage sites that instantiate new PhetioEmitter and see if any of them should be changed to new TinyEmitter.
• [ ] See if any declarations should be : Emitter instead of : PhetioEmitter or : TinyEmitter.
• [ ] Likewise, rename Property to PhetioProperty, probably in a side issue since it will be a lot of work.

Due to the immense amount of work proposed, we would like to confirm with the dev team before starting.

[zepumph]

We discussed this at developer meeting today.

In general, we feel like the Phetio prefix for these central types is not the best choice. While that prefix means something to us, it isn't quite as valuable to describe these types to others (perhaps new members of the project).

We don't feel like this rename would be worth the cost. "bang for the buck" was said about 12 times during our conversation.

We recommend renaming IEmitter -> TEmitter and to be done with this issue.

Likewise, rename Property to PhetioProperty, probably in a side issue since it will be a lot of work.

We feel the same way about the "Phetio" part here. It was also mentioned that we should absolutely not do this until first tackling the larger issue of https://github.com/phetsims/axon/issues/404

• Visit usage sites that instantiate new PhetioEmitter and see if any of them should be changed to new TinyEmitter.

TinyEmitter is not our central emitter, but is only for when we run into performance problems. We as the developers at the 8/4 meeting do not recommend doing this work.

• See if any declarations should be : Emitter instead of : PhetioEmitter or : TinyEmitter.

Every declaration possible should be TEmitter instead of either class (implementation detail).

[samreid]

I renamed IEmitter to TEmitter. I checked for type annotations that should be changed and only saw one.

[samreid]

I think this issue is ready to close. @marlitas can you please spot check? Close if all is well.

[marlitas]

Did a quick spot check, and all looks good. Will add IEmitterListener & IEmitterParameter to our list in: https://github.com/phetsims/chipper/issues/1287 Closing.