Provide a narrowing read-only interface for Property

[samreid]

From phetsims/chipper#1137 (comment) we identified that we would like a way to say that a Property can have its value read and linked but not set.

This may overlap with #343

[samreid]

Eventually we may redo the type hierarchy in #343 but until then this is working great:

declare type ReadonlyProperty<T> = Omit<Property<T>, 'set' | 'setPropertyValue' | 'setInitialValue' | 'reset' | 'dispose' | 'setDeferred'>;

We can add it to axon once axon supports TypeScript. In fact, I think I could add it now since JS sims won't depend on it. Testing...

[samreid]

This patch adds ReadonlyProperty.ts to axon and demonstrates a use case:

Index: main/axon/js/ReadonlyProperty.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/main/axon/js/ReadonlyProperty.ts b/main/axon/js/ReadonlyProperty.ts
new file mode 100644
--- /dev/null	(date 1636523233606)
+++ b/main/axon/js/ReadonlyProperty.ts	(date 1636523233606)
@@ -0,0 +1,11 @@
+// Copyright 2021, University of Colorado Boulder
+import Property from './Property.js';
+
+type getValue<T> = {
+  get value(): T
+};
+type ReadonlyProperty<T> =
+  Omit<Property<T>, 'value' | 'set' | 'setPropertyValue' | 'setInitialValue' | 'reset' | 'dispose' | 'setDeferred'>
+  & getValue<T>;
+
+export default ReadonlyProperty;
\ No newline at end of file
Index: main/bending-light/js/more-tools/view/ChartNode.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/main/bending-light/js/more-tools/view/ChartNode.ts b/main/bending-light/js/more-tools/view/ChartNode.ts
--- a/main/bending-light/js/more-tools/view/ChartNode.ts	(revision 145e8a32029a2b8cad467a99e898a990e14e7ee7)
+++ b/main/bending-light/js/more-tools/view/ChartNode.ts	(date 1636522803498)
@@ -9,6 +9,7 @@
 
 import createObservableArray from '../../../../axon/js/createObservableArray.js';
 import Property from '../../../../axon/js/Property.js';
+import ReadonlyProperty from '../../../../axon/js/ReadonlyProperty.js';
 import Bounds2 from '../../../../dot/js/Bounds2.js';
 import ModelViewTransform2 from '../../../../phetcommon/js/view/ModelViewTransform2.js';
 import Node from '../../../../scenery/js/nodes/Node.js';
@@ -31,7 +32,7 @@
    *                                                                      frames
    * @param {Bounds2} chartBounds - bounds of the chart node
    */
-  constructor( series: Series, modelViewTransformProperty: Property<ModelViewTransform2>, chartBounds: Bounds2 ) {
+  constructor( series: Series, modelViewTransformProperty: ReadonlyProperty<ModelViewTransform2>, chartBounds: Bounds2 ) {
 
     super();
 
Index: main/bending-light/js/more-tools/view/SeriesCanvasNode.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/main/bending-light/js/more-tools/view/SeriesCanvasNode.ts b/main/bending-light/js/more-tools/view/SeriesCanvasNode.ts
--- a/main/bending-light/js/more-tools/view/SeriesCanvasNode.ts	(revision 145e8a32029a2b8cad467a99e898a990e14e7ee7)
+++ b/main/bending-light/js/more-tools/view/SeriesCanvasNode.ts	(date 1636523341941)
@@ -12,10 +12,11 @@
 import bendingLight from '../../bendingLight.js';
 import DataPoint from '../model/DataPoint.js';
 import ModelViewTransform2 from '../../../../phetcommon/js/view/ModelViewTransform2.js';
+import ReadonlyProperty from '../../../../axon/js/ReadonlyProperty.js';
 
 class SeriesCanvasNode extends CanvasNode {
   seriesProperty: Property<DataPoint[]>;
-  modelViewTransformProperty: Property<ModelViewTransform2>;
+  modelViewTransformProperty: ReadonlyProperty<ModelViewTransform2>;
   color: string;
 
   /**
@@ -25,13 +26,20 @@
    * @param {string} color - color of the series
    * @param {Object} [providedOptions] - options that can be passed on to the underlying node
    */
-  constructor( seriesProperty: Property<DataPoint[]>, modelViewTransformProperty: Property<ModelViewTransform2>,
+  constructor( seriesProperty: Property<DataPoint[]>,
+               modelViewTransformProperty: ReadonlyProperty<ModelViewTransform2>,
                color: string, providedOptions?: NodeOptions ) {
 
     super( providedOptions );
     this.seriesProperty = seriesProperty; // @private
     this.modelViewTransformProperty = modelViewTransformProperty; // @private
     this.color = color; // @private
+
+    console.log( modelViewTransformProperty.value );
+    modelViewTransformProperty.link( m => {console.log( m );} );
+    modelViewTransformProperty.reset();
+    modelViewTransformProperty.value = ModelViewTransform2.createIdentity();
+    modelViewTransformProperty.setValue( ModelViewTransform2.createIdentity() );
   }
 
   /**

For convenience, here is the type definition ReadonlyProperty.ts:

// Copyright 2021, University of Colorado Boulder
import Property from './Property.js';

type getValue<T> = {
  get value(): T
};
type ReadonlyProperty<T> =
  Omit<Property<T>, 'value' | 'set' | 'setPropertyValue' | 'setInitialValue' | 'reset' | 'dispose' | 'setDeferred'>
  & getValue<T>;

export default ReadonlyProperty;

And a screenshot showing how you can get values and link but not reset or change the values:

It appears that adding this TypeScript file to axon would not disrupt any pure JavaScript sims, so I think it is safe to add despite our "no TypeScript in common code repos yet" policy. This is because this new file would only be used by TypeScript simulations (since it is a TypeScript *.ts type declaration). For JavaScript sims, it would neither be picked up in the webpack step nor would it require a tsc step. However, I think it would be best to get feedback and approval before adding something like this to common code. @pixelzoom and/or @zepumph can you please review and advise?

[pixelzoom]

Re:

> type ReadonlyProperty<T> =
+  Omit<Property<T>, 'value' | 'set' | 'setPropertyValue' | 'setInitialValue' | 'reset' | 'dispose' | 'setDeferred'>
+  & getValue<T>;

I guess I can see how this might be considered OK from a structural-typing perspective. But from an OOP perspective, it's backwards - ReadonlyProperty would be the base class, and a "writeable" Property would extend it to make it mutable.

Re use cases... I see how declaring a constructor/method parameter as ReadOnlyProperty can be used -- you can pass in a Property argument, and the client is limited to the subset of the API that deals with observing and reading the Property (the API is narrowed). I'm not clear on how ReadOnlyProperty would be used in the more common case, where a class has a Property field that is private/protected writable, but public read-only. Is something like this what you had in mind? :

class Thermometer {

  // Writeable by this class and its subclasses.
  protected readonly temperatureProperty: NumberProperty;
  
  // Requires an additional public reference to someProperty that is read-only ??
  public readonly temperatureReadOnlyProperty: ReadOnlyProperty<number>;

  constructor(...) {
    this.temperatureProperty = new NumberProperty(..., {
      range: new Range (...),
      ...
    } );
    this. temperatureReadOnlyProperty = this.temperatureProperty;
    // constructor and methods can then set this.temperatureProperty
  }
}

// client
const thermometer = new Thermometer();
thermometer.temperatureProperty.link( ... ); // ERROR, private
thermometer.temperatureReadOnlyProperty.link( ... ); // OK

Questions?

• Does this require 2 references to the same Property, as in Thermometer? If so, how should they be named?
  • thermometerProperty and temperatureReadOnlyProperty?
  • thermometerWriteableProperty and temperatureReadOnlyProperty?
  • _temperatureProperty and temperatureProperty?
• How does ReadOnlyProperty declaration work with TinyProperty, NumberProperty, etc, as in temperatureReadOnlyProperty?

[samreid]

I considered 2 strategies for how to allow the class to write to the Property but only allowing clients to read the Property. Strategy A is the one you mentioned above, which seems good to me. An alternative is to only have one field for it, but to provide access via a narrowing getter like so:

Strategy B:

class Thermometer {

  // Writeable by this class and its subclasses.
  protected readonly _temperatureProperty: NumberProperty;

  constructor() {
    this._temperatureProperty = new NumberProperty(0);
    // constructor and methods can then set this._temperatureProperty
  }
  
  get temperatureProperty():ReadOnlyProperty<number>{
    return this._temperatureProperty;
  }
  
  winterFreeze(){
    this._temperatureProperty.set(-100);
  }
}

// client
const thermometer = new Thermometer();
thermometer.temperatureProperty.set( ... ); // Type Error, method does not exist in interface
thermometer.temperatureProperty.link( ()=>{} ); // OK
thermometer.winterFreeze(); // OK

Strategy A: the duplicate class attribute is not great and the naming is confusing.
Strategy B: Requires an underscore and a getter method. Class has to remember the underscored version when changing the value.

Does this require 2 references to the same Property, as in Thermometer? If so, how should they be named?

Let's discuss whether Strategy B is preferable before deciding on the naming for Strategy A.

How does ReadOnlyProperty declaration work with TinyProperty, NumberProperty, etc, as in

I think strategy A or B would work with any Property types or subtypes that match the interface.

New questions:

• In TypeScript "readonly" is one word. Should we make it one word like ReadonlyProperty or two words like ReadOnlyProperty?

[pixelzoom]

I like Strategy B, but...

• How does the client get access to thermometerProperty.rangeProperty?
• How do I ensure that thermometerProperty.rangeProperty is also read-only if I don't want the client to change it?
• In general, how does ReadOnlyProperty work for subclasses of Property, many of which contain additional features that still need to be accessed in a read-only API? And are those additional features also read-only? (all of them? some of them? optionally?...)

[pixelzoom]

In TypeScript "readonly" is one word. Should we make it one word like ReadonlyProperty or two words like ReadOnlyProperty?

Read-only is a compound adjective. And compound adjectives are typically converted to camel case - e.g. "well-behaved three-legged dog" would be wellBehavedThreeLeggedDog, not wellbehavedThreeleggedDog. So I think that TS has it wrong here. And assuming that we're striving for consistency, there would be other consequences of using "readonly" in names. E.g. renaming phetioReadOnly to phetioReadonly will break all PhET-iO APIs, and maybe some PhET-iO client code.

[samreid]

How does the client get access to thermometerProperty.rangeProperty?

We could make a more general ReadOnly generic like:

type ReadOnly<X, T> =
  Omit<X, 'value' | 'set' | 'setPropertyValue' | 'setInitialValue' | 'reset' | 'dispose' | 'setDeferred'>
  & getValue<T>;

Usage would look like:

  get temperatureProperty(): ReadOnly<NumberProperty, number> {
    return this._temperatureProperty;
  }

Call site is OK:

thermometer.temperatureProperty.rangeProperty; // OK

How do I ensure that thermometerProperty.rangeProperty is also read-only if I don't want the client to change it?

Perhaps we would make a way to add more restrictions in the ReadOnly type.

type ReadOnly<X, T, Z extends string> =
  Omit<X, 'value' | 'set' | 'setPropertyValue' | 'setInitialValue' | 'reset' | 'dispose' | 'setDeferred' | Z>
  & getValue<T>;

get temperatureProperty(): ReadOnly<NumberProperty, number, 'rangeProperty'> {

thermometer.temperatureProperty.rangeProperty; // ERROR: rangeProperty does not exist on the type

In general, how does ReadOnlyProperty work for subclasses of Property, many of which contain additional features that still need to be accessed in a read-only API?

The proposal to think of ReadOnly as a generic type that works for any subtype of Property seems like it would work well.

Read-only is a compound adjective. And compound adjectives are typically converted to camel case - e.g. "well-behaved three-legged dog" would be wellBehavedThreeLeggedDog, not wellbehavedThreeleggedDog. So I think that TS has it wrong here.

I agree, we should go with ReadOnly.

New Questions:

• Do you think it is acceptable to add new *.ts code (like this) to common code repos? Pure JavaScript sims will not include this code or require a compilation step, so it won't force JavaScript sims to start using the TypeScript toolchain.
• Is this ReadOnly proposal good enough to commit and start using?

[pixelzoom]

• Do you think it is acceptable to add new *.ts code (like this) to common code repos?

In general, yes.

• Is this ReadOnly proposal good enough to commit and start using? ...

This is a feature that will undoubtedly be used widely, and it seems like "type narrowing" is potentially a good direction. But it's difficult to evaluate whether ReadOnlyPoperty is the future without knowing how will axon change when converted to TS. And there are so many problems with Property et. al, some (many?) of which we hope TS fixes. So my intuition tells me that it would be better to wait until we convert the entire Property stack to TS, and have addressed #343.

[samreid]

The proposal for this issue seems reasonable, but the recommendation is to wait until we have discussed #343. Going on hold until then.

[zepumph]

This issue has been assigned to me to review for a really long time. I'm unsure if it is still the right place for it, but here is a review of axon files being converted to typescript, which seems to have occurred here. Please let me know if there is more that I should do here, or assign me a side issue if this one is on hold or too long.

Review:

Property,ts

• Can we rename Listener2, or do you have a better idea?
• Documentation is needed for the valueOf function

NumberProperty

• Shouldn't we include the Omit in the NumberPropertyOptions? Also why do we need an empty object to "and" with?

  axon/js/NumberProperty.ts

  Lines 40 to 41 in 6e24342

  	type NumberPropertyOptions = {
  	} & Partial<NumberPropertyDefinedOptions> & PropertyOptions<number>;

  I would think it should be instead: type NumberPropertyOptions = Partial<NumberPropertyDefinedOptions> & Omit<PropertyOptions<number>, 'phetioType'>;
• Once we get options patterns set for NumberProperty, it seems like many of the assertions are redundant. Right now webstorm is marking them as redundant.
• Should we discuss deprecating validatorDef in some way, it seems redundant to Typescript for NumberProperty in a couple ways. Perhaps another issue to discuss this if you agree @samreid.

I don't have any comments that I can see for DerivedProperty, except that it the Types declared at the top are really awesome. Perhaps as I use it more I will have thoughts, but right now I am mostly impressed with how seamless it feels with the current implementation.

[samreid]

Regarding the review in the preceding comment, as far as I can tell all those issues have been resolved in other work.

We no longer need the generic described in #363 (comment).

We have discussed Strategy B from #363 (comment) in dev meeting, it seems useful sometimes.

@zepumph and @pixelzoom can you think of remaining work for this issue? Can it be closed?

[zepumph]

Not for me!

[pixelzoom]

Me neither. Closing.