Convert remaining files to typescript

[zepumph]

There are multiple tests and smaller files that are still in javascript. I would say we don't need to convert any files that are deprecated, like EnumerationDeprecatedProperty and its tests.

[zepumph]

@marlitas, are you interested in this?

[marlitas]

PropertyStateHandler unit tests are giving me some problems. This patch is passing tsc, but not QUnit, and I am not sure how to move forward. Requesting help from @zepumph when he has the chance.

```diff Index: js/ReadOnlyProperty.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/ReadOnlyProperty.ts b/js/ReadOnlyProperty.ts --- a/js/ReadOnlyProperty.ts (revision dadfd965d02c403a5ecac3f1f51d6e59870e47a3) +++ b/js/ReadOnlyProperty.ts (date 1668207628537) @@ -101,7 +101,7 @@ // while deferred, new values neither take effect nor send notifications. When isDeferred changes from // true to false, the final deferred value becomes the Property value. An action is created which can be invoked to // send notifications. - protected isDeferred: boolean; + public isDeferred: boolean; // the value that this Property will take after no longer deferred protected deferredValue: T | null; Index: js/PropertyStateHandlerTests.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/PropertyStateHandlerTests.js b/js/PropertyStateHandlerTests.ts rename from js/PropertyStateHandlerTests.js rename to js/PropertyStateHandlerTests.ts --- a/js/PropertyStateHandlerTests.js (revision dadfd965d02c403a5ecac3f1f51d6e59870e47a3) +++ b/js/PropertyStateHandlerTests.ts (date 1668209296652) @@ -34,43 +34,43 @@ assert.ok( phetioStateEngine, 'to avoid eslint no new as side-effects' ); - const propertyA = new BooleanProperty( false, { + const aProperty = new BooleanProperty( false, { tandem: Tandem.ROOT_TEST.createTandem( 'aProperty' ) } ); - const propertyB = new BooleanProperty( true, { + const bProperty = new BooleanProperty( true, { tandem: Tandem.ROOT_TEST.createTandem( 'bProperty' ) } ); - const propertyC = new BooleanProperty( false, { + const cProperty = new BooleanProperty( false, { tandem: Tandem.ROOT_TEST.createTandem( 'cProperty' ) } ); const originalOrderDependencyLength = propertyStateHandler.getNumberOfOrderDependencies(); const getOrderDependencyLength = () => propertyStateHandler.getNumberOfOrderDependencies() - originalOrderDependencyLength; - propertyStateHandler.registerPhetioOrderDependency( propertyA, PropertyStatePhase.UNDEFER, propertyB, PropertyStatePhase.NOTIFY ); + propertyStateHandler.registerPhetioOrderDependency( aProperty, PropertyStatePhase.UNDEFER, bProperty, PropertyStatePhase.NOTIFY ); assert.ok( getOrderDependencyLength() === 1, 'one expected' ); - propertyStateHandler.registerPhetioOrderDependency( propertyA, PropertyStatePhase.UNDEFER, propertyC, PropertyStatePhase.NOTIFY ); + propertyStateHandler.registerPhetioOrderDependency( aProperty, PropertyStatePhase.UNDEFER, cProperty, PropertyStatePhase.NOTIFY ); assert.ok( getOrderDependencyLength() === 2, 'two expected' ); - propertyStateHandler.registerPhetioOrderDependency( propertyB, PropertyStatePhase.UNDEFER, propertyC, PropertyStatePhase.NOTIFY ); + propertyStateHandler.registerPhetioOrderDependency( bProperty, PropertyStatePhase.UNDEFER, cProperty, PropertyStatePhase.NOTIFY ); assert.ok( getOrderDependencyLength() === 3, 'three expected' ); - propertyStateHandler.unregisterOrderDependenciesForProperty( propertyA ); + propertyStateHandler.unregisterOrderDependenciesForProperty( aProperty ); assert.ok( getOrderDependencyLength() === 1, 'a was in two' ); - propertyStateHandler.unregisterOrderDependenciesForProperty( propertyB ); + propertyStateHandler.unregisterOrderDependenciesForProperty( bProperty ); assert.ok( getOrderDependencyLength() === 0, 'none now' ); - propertyStateHandler.registerPhetioOrderDependency( propertyA, PropertyStatePhase.UNDEFER, propertyC, PropertyStatePhase.NOTIFY ); - propertyStateHandler.registerPhetioOrderDependency( propertyB, PropertyStatePhase.UNDEFER, propertyC, PropertyStatePhase.NOTIFY ); + propertyStateHandler.registerPhetioOrderDependency( aProperty, PropertyStatePhase.UNDEFER, cProperty, PropertyStatePhase.NOTIFY ); + propertyStateHandler.registerPhetioOrderDependency( bProperty, PropertyStatePhase.UNDEFER, cProperty, PropertyStatePhase.NOTIFY ); assert.ok( getOrderDependencyLength() === 2, 'none now' ); - propertyStateHandler.unregisterOrderDependenciesForProperty( propertyC ); + propertyStateHandler.unregisterOrderDependenciesForProperty( cProperty ); assert.ok( getOrderDependencyLength() === 0, 'none now' ); - propertyA.dispose(); - propertyB.dispose(); - propertyC.dispose(); + aProperty.dispose(); + bProperty.dispose(); + cProperty.dispose(); if ( window.assert ) { const uninstrumentedProperty = new Property( 2 ); @@ -78,10 +78,12 @@ tandem: Tandem.ROOT_TEST.createTandem( 'instrumentedProperty' ) } ); assert.throws( () => { + propertyStateHandler.registerPhetioOrderDependency( uninstrumentedProperty, PropertyStatePhase.UNDEFER, instrumentedProperty, PropertyStatePhase.UNDEFER ); }, 'cannot register with an uninstrumented Property' ); assert.throws( () => { + propertyStateHandler.registerPhetioOrderDependency( instrumentedProperty, PropertyStatePhase.UNDEFER, instrumentedProperty, PropertyStatePhase.UNDEFER ); }, 'same Property same phase. . . . no no.' ); } @@ -97,7 +99,7 @@ const numberProperty = new NumberProperty( 0, { tandem: Tandem.ROOT_TEST.createTandem( 'numberProperty' ), phetioDynamicElement: true, - range: rangeProperty + range: rangeProperty.value } ); const randomDependencyProperty = new BooleanProperty( false, { @@ -113,9 +115,9 @@ ); const serializedValue = NumberProperty.NumberPropertyIO.toStateObject( numberProperty ); - serializedValue.range.min = 4; - serializedValue.range.max = 8; - serializedValue.value = 7; + serializedValue.range!.min = 4; + serializedValue.range!.max = 8; + serializedValue[ 'value' ] = 7; phet.phetio.phetioEngine.phetioStateEngine.setState( { 'axon.test.numberProperty': serializedValue, @@ -144,33 +146,33 @@ } ); assert.ok( phetioStateEngine, 'to avoid eslint no new as side-effects' ); - const propertyA = new BooleanProperty( false, { + const aProperty = new BooleanProperty( false, { tandem: Tandem.ROOT_TEST.createTandem( 'aProperty' ) } ); - const propertyB = new BooleanProperty( true, { + const bProperty = new BooleanProperty( true, { tandem: Tandem.ROOT_TEST.createTandem( 'bProperty' ) } ); - const propertyC = new BooleanProperty( false, { + const cProperty = new BooleanProperty( false, { tandem: Tandem.ROOT_TEST.createTandem( 'cProperty' ) } ); - propertyStateHandler.registerPhetioOrderDependency( propertyA, PropertyStatePhase.UNDEFER, propertyB, PropertyStatePhase.NOTIFY ); - propertyStateHandler.unregisterOrderDependenciesForProperty( propertyB ); - assert.ok( propertyStateHandler.undeferBeforeNotifyMapPair.beforeMap.size === 0, 'empty entries should be cleared' ); + propertyStateHandler.registerPhetioOrderDependency( aProperty, PropertyStatePhase.UNDEFER, bProperty, PropertyStatePhase.NOTIFY ); + propertyStateHandler.unregisterOrderDependenciesForProperty( bProperty ); + assert.ok( propertyStateHandler[ 'undeferBeforeNotifyMapPair' ].beforeMap.size === 0, 'empty entries should be cleared' ); - propertyStateHandler.registerPhetioOrderDependency( propertyA, PropertyStatePhase.UNDEFER, propertyB, PropertyStatePhase.NOTIFY ); - propertyStateHandler.registerPhetioOrderDependency( propertyA, PropertyStatePhase.UNDEFER, propertyC, PropertyStatePhase.NOTIFY ); - propertyStateHandler.unregisterOrderDependenciesForProperty( propertyA ); - assert.ok( propertyStateHandler.undeferBeforeNotifyMapPair.beforeMap.size === 0, 'empty entries should be cleared' ); + propertyStateHandler.registerPhetioOrderDependency( aProperty, PropertyStatePhase.UNDEFER, bProperty, PropertyStatePhase.NOTIFY ); + propertyStateHandler.registerPhetioOrderDependency( aProperty, PropertyStatePhase.UNDEFER, cProperty, PropertyStatePhase.NOTIFY ); + propertyStateHandler.unregisterOrderDependenciesForProperty( aProperty ); + assert.ok( propertyStateHandler[ 'undeferBeforeNotifyMapPair' ].beforeMap.size === 0, 'empty entries should be cleared' ); - propertyStateHandler.registerPhetioOrderDependency( propertyA, PropertyStatePhase.UNDEFER, propertyB, PropertyStatePhase.NOTIFY ); - propertyStateHandler.registerPhetioOrderDependency( propertyA, PropertyStatePhase.UNDEFER, propertyC, PropertyStatePhase.NOTIFY ); - propertyStateHandler.unregisterOrderDependenciesForProperty( propertyB ); - propertyStateHandler.unregisterOrderDependenciesForProperty( propertyC ); - assert.ok( propertyStateHandler.undeferBeforeNotifyMapPair.beforeMap.size === 0, 'empty entries should be cleared' ); + propertyStateHandler.registerPhetioOrderDependency( aProperty, PropertyStatePhase.UNDEFER, bProperty, PropertyStatePhase.NOTIFY ); + propertyStateHandler.registerPhetioOrderDependency( aProperty, PropertyStatePhase.UNDEFER, cProperty, PropertyStatePhase.NOTIFY ); + propertyStateHandler.unregisterOrderDependenciesForProperty( bProperty ); + propertyStateHandler.unregisterOrderDependenciesForProperty( cProperty ); + assert.ok( propertyStateHandler[ 'undeferBeforeNotifyMapPair' ].beforeMap.size === 0, 'empty entries should be cleared' ); - propertyA.dispose(); - propertyB.dispose(); - propertyC.dispose(); + aProperty.dispose(); + bProperty.dispose(); + cProperty.dispose(); } ); } \ No newline at end of file Index: js/PropertyStateHandler.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/PropertyStateHandler.js b/js/PropertyStateHandler.ts rename from js/PropertyStateHandler.js rename to js/PropertyStateHandler.ts --- a/js/PropertyStateHandler.js (revision dadfd965d02c403a5ecac3f1f51d6e59870e47a3) +++ b/js/PropertyStateHandler.ts (date 1668209371112) @@ -9,32 +9,50 @@ * @author Michael Kauzmann (PhET Interactive Simulations) */ +import PhetioStateEngine from '../../phet-io/js/PhetioStateEngine.js'; import Tandem from '../../tandem/js/Tandem.js'; import axon from './axon.js'; import PropertyStatePhase from './PropertyStatePhase.js'; import ReadOnlyProperty from './ReadOnlyProperty.js'; +type PhaseMap = { + beforePhase?: PropertyStatePhase; + afterPhase?: PropertyStatePhase; + otherMap?: PhaseMap; + beforePhetioID?: Set; + afterPhetioID?: Set; +} & Map>; + +type RelevantOrderDependencies = { + beforeTerm: string; + afterTerm: string; +}; + class PropertyStateHandler { + private readonly phaseCallbackSets: PhaseCallbackSets; + private readonly undeferBeforeUndeferMapPair: OrderDependencyMapPair; + private readonly undeferBeforeNotifyMapPair: OrderDependencyMapPair; + private readonly notifyBeforeUndeferMapPair: OrderDependencyMapPair; + private readonly notifyBeforeNotifyMapPair: OrderDependencyMapPair; + private readonly mapPairs: OrderDependencyMapPair[]; + public initialized: boolean; /** - * @param {PhetioStateEngine|undefined} phetioStateEngine - not provided for tests + * {PhetioStateEngine|undefined} phetioStateEngine - not provided for tests */ - constructor( phetioStateEngine ) { + public constructor( phetioStateEngine?: PhetioStateEngine ) { // Properties support setDeferred(). We defer setting their values so all changes take effect // at once. This keeps track of finalization actions (embodied in a PhaseCallback) that must take place after all // Property values have changed. This keeps track of both types of PropertyStatePhase: undeferring and notification. - // @private {Set.} this.phaseCallbackSets = new PhaseCallbackSets(); - // @private - each pair has a Map optimized for looking up based on the "before phetioID" and the "after phetioID" // of the dependency. Having a data structure set up for both directions of look-up makes each operation O(1). See https://github.com/phetsims/axon/issues/316 this.undeferBeforeUndeferMapPair = new OrderDependencyMapPair( PropertyStatePhase.UNDEFER, PropertyStatePhase.UNDEFER ); this.undeferBeforeNotifyMapPair = new OrderDependencyMapPair( PropertyStatePhase.UNDEFER, PropertyStatePhase.NOTIFY ); this.notifyBeforeUndeferMapPair = new OrderDependencyMapPair( PropertyStatePhase.NOTIFY, PropertyStatePhase.UNDEFER ); this.notifyBeforeNotifyMapPair = new OrderDependencyMapPair( PropertyStatePhase.NOTIFY, PropertyStatePhase.NOTIFY ); - // @private - keep a list of all map pairs for easier iteration this.mapPairs = [ this.undeferBeforeUndeferMapPair, this.undeferBeforeNotifyMapPair, @@ -42,15 +60,10 @@ this.notifyBeforeNotifyMapPair ]; - // @public (PropertyStateHandlerTests read-only) this.initialized = false; } - /** - * @param {PhetioStateEngine} phetioStateEngine - * @public - */ - initialize( phetioStateEngine ) { + public initialize( phetioStateEngine: PhetioStateEngine ): void { assert && assert( !this.initialized, 'cannot initialize twice' ); phetioStateEngine.onBeforeApplyStateEmitter.addListener( phetioObject => { @@ -85,32 +98,18 @@ this.initialized = true; } - /** - * @private - * @param {ReadOnlyProperty} property - */ - validateInstrumentedProperty( property ) { + private static validateInstrumentedProperty( property: ReadOnlyProperty ): void { assert && Tandem.VALIDATION && assert( property instanceof ReadOnlyProperty && property.isPhetioInstrumented(), `must be an instrumented Property: ${property}` ); } - /** - * @private - * @param {Property} property - * @param {PropertyStatePhase} phase - */ - validatePropertyPhasePair( property, phase ) { - this.validateInstrumentedProperty( property ); - assert && assert( phase instanceof PropertyStatePhase, `unexpected phase: ${phase}` ); + private validatePropertyPhasePair( property: ReadOnlyProperty, phase: PropertyStatePhase ): void { + PropertyStateHandler.validateInstrumentedProperty( property ); } /** * Get the MapPair associated with the proved PropertyStatePhases - * @private - * @param {PropertyStatePhase} beforePhase - * @param {PropertyStatePhase} afterPhase - * @returns {OrderDependencyMapPair} */ - getMapPairFromPhases( beforePhase, afterPhase ) { + private getMapPairFromPhases( beforePhase: PropertyStatePhase, afterPhase: PropertyStatePhase ): OrderDependencyMapPair { const matchedPairs = this.mapPairs.filter( mapPair => beforePhase === mapPair.beforePhase && afterPhase === mapPair.afterPhase ); assert && assert( matchedPairs.length === 1, 'one and only one map should match the provided phases' ); return matchedPairs[ 0 ]; @@ -121,19 +120,15 @@ * is an ending state in PhET-iO state set where Property values solidify, notifications for value changes are called. * The PhET-iO state engine will always undefer a Property before it notifies its listeners. This is for registering * two different Properties. - * @public * - * @param {ReadOnlyProperty} beforeProperty - the Property that needs to be set before the second; must be instrumented for PhET-iO - * @param {PropertyStatePhase} beforePhase - * @param {ReadOnlyProperty} afterProperty - must be instrumented for PhET-iO - * @param {PropertyStatePhase} afterPhase */ - registerPhetioOrderDependency( beforeProperty, beforePhase, afterProperty, afterPhase ) { + public registerPhetioOrderDependency( beforeProperty: ReadOnlyProperty, beforePhase: PropertyStatePhase, afterProperty: ReadOnlyProperty, afterPhase: PropertyStatePhase ): void { if ( Tandem.PHET_IO_ENABLED ) { this.validatePropertyPhasePair( beforeProperty, beforePhase ); this.validatePropertyPhasePair( afterProperty, afterPhase ); - assert && beforeProperty === afterProperty && assert( beforePhase !== afterPhase, 'cannot set same Property to same phase' ); + + assert && assert( beforePhase !== afterPhase, 'cannot set same Property to same phase' ); const mapPair = this.getMapPairFromPhases( beforePhase, afterPhase ); @@ -142,23 +137,21 @@ } /** - * @param {Property} property - must be instrumented for PhET-iO - * @returns {boolean} - true if Property is in any order dependency - * @private + * {Property} property - must be instrumented for PhET-iO + * {boolean} - true if Property is in any order dependency */ - propertyInAnOrderDependency( property ) { - this.validateInstrumentedProperty( property ); + private propertyInAnOrderDependency( property: ReadOnlyProperty ): boolean { + PropertyStateHandler.validateInstrumentedProperty( property ); return _.some( this.mapPairs, mapPair => mapPair.usesPhetioID( property.tandem.phetioID ) ); } /** * Unregisters all order dependencies for the given Property - * @param {ReadOnlyProperty} property - must be instrumented for PhET-iO - * @public + * {ReadOnlyProperty} property - must be instrumented for PhET-iO */ - unregisterOrderDependenciesForProperty( property ) { + public unregisterOrderDependenciesForProperty( property: ReadOnlyProperty ): void { if ( Tandem.PHET_IO_ENABLED ) { - this.validateInstrumentedProperty( property ); + PropertyStateHandler.validateInstrumentedProperty( property ); // Be graceful if given a Property that is not registered in an order dependency. if ( this.propertyInAnOrderDependency( property ) ) { @@ -172,11 +165,9 @@ /** * Given registered Property Phase order dependencies, undefer all AXON/Property PhET-iO elements to take their * correct values and have each notify their listeners. - * - * @private - * @param {Set.} phetioIDsInState - set of phetioIDs that were set in state + * {Set.} phetioIDsInState - set of phetioIDs that were set in state */ - undeferAndNotifyProperties( phetioIDsInState ) { + private undeferAndNotifyProperties( phetioIDsInState: Set ): void { assert && assert( this.initialized, 'must be initialized before getting called' ); // {Object.} - true if a phetioID + phase pair has been applied, keys are the combination of @@ -201,17 +192,14 @@ } } - /** - * @param {Object.} completedPhases - * @private - */ - errorInUndeferAndNotifyStep( completedPhases ) { + + private errorInUndeferAndNotifyStep( completedPhases: Record ): void { // combine phetioID and Phase into a single string to keep this process specific. - const stillToDoIDPhasePairs = []; + const stillToDoIDPhasePairs: Array = []; this.phaseCallbackSets.forEach( phaseCallback => stillToDoIDPhasePairs.push( phaseCallback.getTerm() ) ); - const relevantOrderDependencies = []; + const relevantOrderDependencies: Array = []; this.mapPairs.forEach( mapPair => { const beforeMap = mapPair.beforeMap; @@ -250,10 +238,9 @@ /** * Only for Testing! * Get the number of order dependencies registered in this class - * @public - * @returns {number} + * */ - getNumberOfOrderDependencies() { + public getNumberOfOrderDependencies(): number { let count = 0; this.mapPairs.forEach( mapPair => { mapPair.afterMap.forEach( valueSet => { count += valueSet.size; } ); @@ -264,13 +251,12 @@ /** * Go through all phases still to be applied, and apply them if the order dependencies allow it. Only apply for the * particular phase provided. In general UNDEFER must occur before the same phetioID gets NOTIFY. - * @private * - * @param {PropertyStatePhase} phase - only apply PhaseCallbacks for this particular PropertyStatePhase - * @param {Object.} completedPhases - map that keeps track of completed phases - * @param {Set.} phetioIDsInState - set of phetioIDs that were set in state + * {PropertyStatePhase} phase - only apply PhaseCallbacks for this particular PropertyStatePhase + * {Object.} completedPhases - map that keeps track of completed phases + * {Set.} phetioIDsInState - set of phetioIDs that were set in state */ - attemptToApplyPhases( phase, completedPhases, phetioIDsInState ) { + private attemptToApplyPhases( phase: PropertyStatePhase, completedPhases: Record, phetioIDsInState: Set ): void { const phaseCallbackSet = this.phaseCallbackSets.getSetFromPhase( phase ); @@ -294,14 +280,13 @@ } /** - * @private - * @param {string} phetioID - think of this as the "afterPhetioID" since there may be some phases that need to be applied before it has this phase done. - * @param {PropertyStatePhase} phase - * @param {Object.} completedPhases - map that keeps track of completed phases - * @param {Set.} phetioIDsInState - set of phetioIDs that were set in state - * @returns {boolean} - if the provided phase can be applied given the dependency order dependencies of the state engine. + * {string} phetioID - think of this as the "afterPhetioID" since there may be some phases that need to be applied before it has this phase done. + * {PropertyStatePhase} phase + * {Object.} completedPhases - map that keeps track of completed phases + * {Set.} phetioIDsInState - set of phetioIDs that were set in state + * {boolean} - if the provided phase can be applied given the dependency order dependencies of the state engine. */ - phetioIDCanApplyPhase( phetioID, phase, completedPhases, phetioIDsInState ) { + private phetioIDCanApplyPhase( phetioID: string, phase: PropertyStatePhase, completedPhases: Record, phetioIDsInState: Set ): boolean { // Undefer must happen before notify if ( phase === PropertyStatePhase.NOTIFY && !completedPhases[ phetioID + PropertyStatePhase.UNDEFER ] ) { @@ -309,7 +294,7 @@ } // Get a list of the maps for this phase being applies. - const mapsToCheck = []; + const mapsToCheck: Array = []; this.mapPairs.forEach( mapPair => { if ( mapPair.afterPhase === phase ) { @@ -327,7 +312,7 @@ const setOfThingsThatShouldComeFirst = mapToCheck.get( phetioID ); // O(K) where K is the number of elements that should come before Property X - for ( const beforePhetioID of setOfThingsThatShouldComeFirst ) { + for ( const beforePhetioID of setOfThingsThatShouldComeFirst! ) { // check if the before phase for this order dependency has already been completed // Make sure that we only care about elements that were actually set during this state set @@ -343,38 +328,38 @@ // POJSO for a callback for a specific Phase in a Property's state set lifecycle. See undeferAndNotifyProperties() class PhaseCallback { + public readonly phetioID; + public readonly phase; + public readonly listener; - /** - * @param {string} phetioID - * @param {function|null} listener - * @param {PropertyStatePhase} phase - */ - constructor( phetioID, listener, phase ) { + public constructor( phetioID: string, listener: ( () => void ) | null, phase: PropertyStatePhase ) { - // @public this.phetioID = phetioID; this.phase = phase; this.listener = listener || _.noop; } /** - * @public - * @returns {string} - unique term for the id/phase pair + * {string} - unique term for the id/phase pair */ - getTerm() { + public getTerm(): string { return this.phetioID + this.phase; } } class OrderDependencyMapPair { + public readonly beforeMap: PhaseMap; + public readonly afterMap: PhaseMap; + public readonly beforePhase; + public readonly afterPhase; + /** - * @param {PropertyStatePhase} beforePhase - * @param {PropertyStatePhase} afterPhase + * {PropertyStatePhase} beforePhase + * {PropertyStatePhase} afterPhase */ - constructor( beforePhase, afterPhase ) { + public constructor( beforePhase: PropertyStatePhase, afterPhase: PropertyStatePhase ) { - // @public (read-only) - fields for mass consumption this.beforeMap = new Map(); this.beforeMap.beforePhase = beforePhase; this.beforeMap.afterPhase = afterPhase; @@ -393,38 +378,33 @@ /** * Register an order dependency between two phetioIDs. This will add data to maps in "both direction". If accessing * with just the beforePhetioID, or with the afterPhetioID. - * @public - * @param {string} beforePhetioID - * @param {string} afterPhetioID */ - addOrderDependency( beforePhetioID, afterPhetioID ) { + public addOrderDependency( beforePhetioID: string, afterPhetioID: string ): void { if ( !this.beforeMap.has( beforePhetioID ) ) { - this.beforeMap.set( beforePhetioID, new Set() ); + this.beforeMap.set( beforePhetioID, new Set() ); } - this.beforeMap.get( beforePhetioID ).add( afterPhetioID ); + this.beforeMap.get( beforePhetioID )!.add( afterPhetioID ); if ( !this.afterMap.has( afterPhetioID ) ) { this.afterMap.set( afterPhetioID, new Set() ); } - this.afterMap.get( afterPhetioID ).add( beforePhetioID ); + this.afterMap.get( afterPhetioID )!.add( beforePhetioID ); } /** * Unregister all order dependencies for the provided Property - * @private - * @param {Property} property */ - unregisterOrderDependenciesForProperty( property ) { + public unregisterOrderDependenciesForProperty( property: ReadOnlyProperty ): void { const phetioIDToRemove = property.tandem.phetioID; [ this.beforeMap, this.afterMap ].forEach( map => { if ( map.has( phetioIDToRemove ) ) { - map.get( phetioIDToRemove ).forEach( phetioID => { - const setOfAfterMapIDs = map.otherMap.get( phetioID ); + map.get( phetioIDToRemove )!.forEach( phetioID => { + const setOfAfterMapIDs = map.otherMap!.get( phetioID ); setOfAfterMapIDs && setOfAfterMapIDs.delete( phetioIDToRemove ); // Clear out empty entries to avoid having lots of empty Sets sitting around - setOfAfterMapIDs.size === 0 && map.otherMap.delete( phetioID ); + setOfAfterMapIDs!.size === 0 && map.otherMap!.delete( phetioID ); } ); } map.delete( phetioIDToRemove ); @@ -439,64 +419,39 @@ } ); } - /** - * @public - * @param {string} phetioID - * @returns {boolean} - */ - usesPhetioID( phetioID ) { + public usesPhetioID( phetioID: string ): boolean { return this.beforeMap.has( phetioID ) || this.afterMap.has( phetioID ); } } // POJSO to keep track of PhaseCallbacks while providing O(1) lookup time because it is built on Set class PhaseCallbackSets { - constructor() { + public readonly undeferSet: Set; + public readonly notifySet: Set; + public constructor() { - // @public (read-only) {Set.} this.undeferSet = new Set(); this.notifySet = new Set(); } - /** - * @public - * @returns {number} - */ - get size() { + public get size(): number { return this.undeferSet.size + this.notifySet.size; } - /** - * @public - * @param {function} callback - */ - forEach( callback ) { + public forEach( callback: ( phaseCallback: PhaseCallback ) => number ): void { this.undeferSet.forEach( callback ); this.notifySet.forEach( callback ); } - /** - * @public - * @param {PhaseCallback} phaseCallback - */ - addUndeferPhaseCallback( phaseCallback ) { + public addUndeferPhaseCallback( phaseCallback: PhaseCallback ): void { this.undeferSet.add( phaseCallback ); } - /** - * @public - * @param {PhaseCallback} phaseCallback - */ - addNotifyPhaseCallback( phaseCallback ) { + public addNotifyPhaseCallback( phaseCallback: PhaseCallback ): void { this.notifySet.add( phaseCallback ); } - /** - * @public - * @param {PropertyStatePhase} phase - * @returns {Set.} - */ - getSetFromPhase( phase ) { + public getSetFromPhase( phase: PropertyStatePhase ): Set { return phase === PropertyStatePhase.NOTIFY ? this.notifySet : this.undeferSet; } } Index: js/ObservableArrayDef.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/ObservableArrayDef.js b/js/ObservableArrayDef.ts rename from js/ObservableArrayDef.js rename to js/ObservableArrayDef.ts --- a/js/ObservableArrayDef.js (revision dadfd965d02c403a5ecac3f1f51d6e59870e47a3) +++ b/js/ObservableArrayDef.ts (date 1668193705309) @@ -7,6 +7,8 @@ */ import axon from './axon.js'; +import { ObservableArray } from './createObservableArray.js'; +import IntentionalAny from '../../phet-core/js/types/IntentionalAny.js'; // TODO https://github.com/phetsims/axon/issues/331 WebStorm is having problems with {ObservableArrayDef} vs {Array} params /** @@ -20,13 +22,10 @@ /** * Returns true if the argument has the properties that an ObservableArrayDef should have. - * @public - * - * @returns {boolean} */ - isObservableArray( observableArray ) { + isObservableArray( observableArray: ObservableArray ): boolean { + return !!( Array.isArray( observableArray ) && observableArray.elementAddedEmitter && observableArray.elementRemovedEmitter && observableArray.lengthProperty ); - return Array.isArray( observableArray ) && observableArray.elementAddedEmitter && observableArray.elementRemovedEmitter && observableArray.lengthProperty; } }; diff --git a/js/propertyStateHandlerSingleton.js b/js/propertyStateHandlerSingleton.ts rename from js/propertyStateHandlerSingleton.js rename to js/propertyStateHandlerSingleton.ts ```

[marlitas]

@samreid helped me address the problems I was coming up against, and I can continue to move forward on the Axon conversion. Thanks @samreid! Un-assigning @zepumph until the rest is done and ready for review.

[marlitas]

In TinyForwardingPropertyTests.ts I used a ts-expect-error for this line:

myForwardingProperty.setTargetProperty( null, null, myDerivedProperty );

Without the ts-expect-error I was receiving an error about DerivedProperty's setter being protected, and it made me wonder if that line is even allowed, and if it is why? @zepumph can you check that for me and provide some feedback?

[zepumph]

You were definitely on to something. This is a weird one. Please read the doc and extra test I added above and let me know if you have any questions.

[marlitas]

@zepumph that makes way more sense. Thanks for the clarification.

Onwards!

[marlitas]

This is completed and ready for review. I left deprecated files as js, as well as axon-phet-io-overrides, wasn't sure if that should be converted since it's a pre-load? Let me know if there's any additional work that should be done there.

Otherwise, assigning to @zepumph for review.

[zepumph]

RE: https://github.com/phetsims/axon/commit/749cf4bedc4d60629a313286a7a0b2ab20f79696

• I'd like to see us keep specificity when possible in unit tests. It is a small thing, but it is definitely better to make sure that that is true, and not just any truthy, javascript-casted boolean (same with https://github.com/phetsims/axon/commit/749cf4bedc4d60629a313286a7a0b2ab20f79696):

Subject: [PATCH] support multiple digits of major.minor versions, https://github.com/phetsims/phet-io-wrappers/issues/473
---
Index: js/TinyForwardingPropertyTests.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/TinyForwardingPropertyTests.ts b/js/TinyForwardingPropertyTests.ts
--- a/js/TinyForwardingPropertyTests.ts (revision 8a8debc476567daee8b1571b0a6df80eee1b678c)
+++ b/js/TinyForwardingPropertyTests.ts (date 1672765981026)
@@ -18,7 +18,7 @@

   const myForwardingProperty = new TinyForwardingProperty<unknown>( true, false );

-  assert.ok( myForwardingProperty.get(), 'basic value for Property' );
+  assert.ok( myForwardingProperty.get() === true, 'basic value for Property' );

   const myTinyProperty = new TinyProperty( 'tinyProperty' );
   myForwardingProperty.setTargetProperty( null, null, myTinyProperty );

• I wonder if <unknown> is best here. I'd rather keep specific typing that works, rather than purposefully transform the Property type into a less-specific type in order to pass type checking. I think that new TinyForwardingProperty<boolean | string | number> works best here.

RE: https://github.com/phetsims/axon/blob/071b221e1cae2574a120cd4420d9c22a52f11da7/js/TinyEmitterTests.ts#L35

How about just passing in _.noop as the function?

Subject: [PATCH] support multiple digits of major.minor versions, https://github.com/phetsims/phet-io-wrappers/issues/473
---
Index: js/TinyEmitterTests.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/TinyEmitterTests.ts b/js/TinyEmitterTests.ts
--- a/js/TinyEmitterTests.ts    (revision 8a8debc476567daee8b1571b0a6df80eee1b678c)
+++ b/js/TinyEmitterTests.ts    (date 1672766395640)
@@ -32,7 +32,7 @@
   e2.emit( '2, 2' );
   e2.emit( undefined );
   e2.emit( null );
-  e2.emit( new TinyEmitter(), 7, () => { _.noop(); } );
+  e2.emit( new TinyEmitter(), 7, _.noop );
   e2.emit( new TinyEmitter() );
 } );

• I made lots of changes to PropertyStateHandler, let's talk about them! Some are my opinions, some are because I have knowledge about the inner workings of the file, and others are questions/guesses to discuss with you. What do you think?

Thanks for all this great work!

[marlitas]

@zepumph thanks for the review notes! I applied the changes to you described and also looked through the changes you added in PropertyStateHandler.

I do have a question with the PropertyStateHandler related to the benefits of generics vs. unknown. I saw you switched out many of the generics to be unknown and I thought that generics were safer overall, but perhaps I'm misunderstanding how they function. Let's find a time to chat after your honeymoon!

[samreid]

I saw the changes like converting

  public unregisterOrderDependenciesForProperty<T>( property: ReadOnlyProperty<T> ): void {
    const phetioIDToRemove = property.tandem.phetioID;

to

  public unregisterOrderDependenciesForProperty( property: ReadOnlyProperty<unknown> ): void {
    const phetioIDToRemove = property.tandem.phetioID;

In this case, the generic type parameter was providing no additional type checking or benefit, because it doesn't matter what the type parameter is. So it is clearer to say "unknown" so that it can accept any type. All of the public interface methods in PropertyStateHandler accept ReadOnlyProperty<IntentionalAny>, and all of the private ones use ReadOnlyProperty<unknown> since they are already any by that point. The exception is public unregisterOrderDependenciesForProperty( property: ReadOnlyProperty<unknown> ): void { but note it is local to that file only (careful, there are 2x of those methods, in different classes).

So I guess all of the ReadOnlyProperty<unknown> in this file could be written as ReadOnlyProperty<IntentionalAny>, but a rule of thumb is to prefer unknown to IntentionalAny where possible.

[marlitas]

Met with @samreid to clarify some of my follow up questions from the above comment. I now have a much stronger understanding of generics and unknown, as well as the preferred use case for each. Thanks @samreid!

Closing this issue as completed.

[phet-dev]

Reopening because there is a TODO marked for this issue.