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

ReadOnlyProperty should have isSettable: false by default #411

Closed samreid closed 1 year ago

samreid commented 1 year ago

@marlitas and I were shocked to see that ReadOnlyProperty specifies:

  /**
   * Returns true if the value can be set externally, using .value= or set()
   */
  public isSettable(): boolean {
    return true;
  }

This sounds very incorrect. We would like to change it to false.

marlitas commented 1 year ago

Attempted patch.

```diff Index: js/Property.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/Property.ts b/js/Property.ts --- a/js/Property.ts (revision a5961582e9fed7de7a0df002257961872cec4d7c) +++ b/js/Property.ts (date 1661358900305) @@ -64,6 +64,13 @@ public override set( value: T ): void { super.set( value ); } + + /** + * Returns true if the value can be set externally, using .value= or set() + */ + public override isSettable(): boolean { + return true; + } } export type { PropertyOptions }; Index: js/TinyForwardingProperty.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/TinyForwardingProperty.ts b/js/TinyForwardingProperty.ts --- a/js/TinyForwardingProperty.ts (revision a5961582e9fed7de7a0df002257961872cec4d7c) +++ b/js/TinyForwardingProperty.ts (date 1661359790802) @@ -63,6 +63,10 @@ * @returns the passed in Node, for chaining. */ public setTargetProperty( node: NodeType, tandemName: string | null, newTargetProperty: TProperty | null ): NodeType { + // We're not really sure about this. + if ( this.targetProperty ) { + assert && assert( this.targetProperty.isSettable(), 'Target Property not settable' ); + } assert && node && tandemName === null && assert( !node.isPhetioInstrumented(), 'tandemName must be provided for instrumented Nodes' ); // no-op if we are already forwarding to that property OR if we still aren't forwarding 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 a5961582e9fed7de7a0df002257961872cec4d7c) +++ b/js/ReadOnlyProperty.ts (date 1661358676129) @@ -199,7 +199,7 @@ * Returns true if the value can be set externally, using .value= or set() */ public isSettable(): boolean { - return true; + return false; } /** ```
samreid commented 1 year ago

There was another incorrect value in DynamicProperty. Fixed and local aqua is looking good so far, so I committed. Closing.