phetsims / projectile-data-lab

"Projectile Data Lab" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
0 stars 0 forks source link

DynamicProperty field declarations that should be TReadOnlyProperty #222

Closed pixelzoom closed 5 months ago

pixelzoom commented 5 months ago

For code review #32 ...

There are many declarations of public fields of type DynamicProperty that should be TReadOnlyProperty. Search for ": DynamicProperty" and inspect them.

For example, in Field.ts:

-  public readonly meanSpeedProperty: DynamicProperty<number, number, Launcher>;
+  public readonly meanSpeedProperty: TReadOnlyProperty<number>;

In some cases, like SMField, this pattern should be used:

SMField patch ```diff Subject: [PATCH] replace 't' with 'launcher' or 'field', https://github.com/phetsims/projectile-data-lab/issues/220 --- Index: js/common-sm/model/SMField.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/common-sm/model/SMField.ts b/js/common-sm/model/SMField.ts --- a/js/common-sm/model/SMField.ts (revision f269a55ec25131b4c17760a63c054b5720980104) +++ b/js/common-sm/model/SMField.ts (date 1709508695471) @@ -14,26 +14,29 @@ import projectileDataLab from '../../projectileDataLab.js'; import { VSMFieldIdentifier } from '../../common-vsm/model/VSMFieldIdentifier.js'; import { EmptySelfOptions } from '../../../../phet-core/js/optionize.js'; +import { TReadOnlyProperty } from '../../../../axon/js/imports.js'; type SelfOptions = EmptySelfOptions; export type SMFieldOptions = SelfOptions & VSMFieldOptions; export default class SMField extends VSMField { - public readonly customLauncherMechanismProperty: DynamicProperty; + public readonly customLauncherMechanismProperty: TReadOnlyProperty; + private readonly _customLauncherMechanismProperty: DynamicProperty; public constructor( launchers: readonly Launcher[], identifier: VSMFieldIdentifier, providedOptions: SMFieldOptions ) { super( launchers, identifier, providedOptions ); - this.customLauncherMechanismProperty = new DynamicProperty( this.launcherProperty, { + this._customLauncherMechanismProperty = new DynamicProperty( this.launcherProperty, { bidirectional: true, derive: launcher => launcher.launcherMechanismProperty } ); + this.customLauncherMechanismProperty = this._customLauncherMechanismProperty; } public override reset(): void { super.reset(); - this.customLauncherMechanismProperty.reset(); + this._customLauncherMechanismProperty.reset(); } } ```
samreid commented 5 months ago

@matthew-blackman and I aren't too excited about using double declarations to solve the access issues. We made one change above and @samreid will review the other occurrences to see how to proceed.

samreid commented 5 months ago

OK we addressed the remainder of the cases. We left a few DynamicProperty type declarations where there wasn't a bundle of other related dynamic properties or where the API was mostly necessary/valuable.

Closing.