Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DynamicProperty field declarations that should be TReadOnlyProperty #222

Closed
pixelzoom opened this issue Mar 3, 2024 · 2 comments
Closed
Assignees

Comments

@pixelzoom
Copy link
Contributor

pixelzoom commented Mar 3, 2024

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
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<LauncherMechanism, LauncherMechanism, Launcher>;
+  public readonly customLauncherMechanismProperty: TReadOnlyProperty<LauncherMechanism>;
+  private readonly _customLauncherMechanismProperty: DynamicProperty<LauncherMechanism, LauncherMechanism, Launcher>;
 
   public constructor( launchers: readonly Launcher[], identifier: VSMFieldIdentifier, providedOptions: SMFieldOptions ) {
     super( launchers, identifier, providedOptions );
 
-    this.customLauncherMechanismProperty = new DynamicProperty<LauncherMechanism, LauncherMechanism, Launcher>( this.launcherProperty, {
+    this._customLauncherMechanismProperty = new DynamicProperty<LauncherMechanism, LauncherMechanism, Launcher>( 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
Copy link
Member

samreid commented Mar 4, 2024

@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
Copy link
Member

samreid commented Mar 6, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants