addLinkedElement is causing APIs to use Property instead of IProperty/IReadOnlyProperty

pixelzoom commented 2 years ago

Slack discussion:

Chris Malley 11:59 AM As I’m doing these conversions, it makes me sad that I have to type parameters as Property instead of IProperty because some ancestor class calls addLinkedElement and therefore requires a PhetioObject. Typing as Property provides access to things that should not be used, like reset .

Michael Kauzmann:palm_tree: 1:07 PM I feel like all scenery-phet and sun components should only take a Property (or an interface based on it). I wouldn't think it ideal to have 10 components that don't yet have a LinkedElement because they aren't PhET-iO instrumented, but they take an IProperty and in a couple months have usages that pass it a TinyProperty. They should be Properties for iO support at this time.

Chris Malley 2:00 PM Why should they be Property (and expose the entire Property API, including reset ) simply because they need to be passed to addLinkedElement?

Sam Reid 2:13 PM What about specifying the minimal interface used?

type CheckboxProperty = {
  set value( value: boolean );
  link( listener: PropertyLinkListener<boolean>, options?: any ): void;
  unlink( listener: PropertyListener<boolean> ): void;
  hasListener( listener: PropertyLinkListener<boolean> ): boolean;
} & PhetioObject;

// Checkbox
constructor( content: Node, property: CheckboxProperty, providedOptions?: CheckboxOptions ) {

2:14 Or, to avoid repeating ourselves:

[2:14](https://phetsims.slack.com/archives/C029UG5BVU3/p1647461698854479)
type CheckboxProperty = Pick<Property<boolean>, 'value' | 'link' | 'unlink' | 'hasListener'> & PhetioObject;

Chris Malley 2:18 PM I think this is a problem with IReadOnlyProperty. It omits the PhetioObject-ness of read-only Properties. 2:22 As far as I can tell addLinkedElement doesn’t required anything about its “element” to be mutable. But it’s loaded with dependencies on PhetioObject. Can some part of PhetioObject be factored out as IReadOnlyPhetioObject ? And then type IReadOnlyProperty = { … } & IReadOnlyPhetioObject;

Sam Reid 2:29 PM Here’s all we need to know about linked elements:

export type LinkableElement = Pick<PhetioObject, 'phetioFeatured' | 'isPhetioInstrumented'>;

2:29 Then the signature can read:

addLinkedElement( element: LinkableElement, options?: any ): void {

jbphet commented 2 years ago

I just ran into an example case of this. I had this code in AmplitudeModulatorDemoNode.js

    // LFO enabled control
    const lfoEnabled = new Checkbox(
      new Text( 'LFO Enabled', { font: LABEL_FONT } ),
      amplitudeModulatorDemo.amplitudeModulator.enabledProperty,
      { boxWidth: 16 }
    );

The enabledProperty on the modulator is present because it extends EnabledComponent, and the type for it is IProperty<boolean>. When I changed AmplitudeModulatorDemoNode to TypeScript, my IDE was complaining that the type for the enabledProperty was incorrect, so I ended up adding as BooleanProperty like this:

    // LFO enabled control
    const lfoEnabled = new Checkbox(
      new Text( 'LFO Enabled', { font: LABEL_FONT } ),
      amplitudeModulatorDemo.amplitudeModulator.enabledProperty as BooleanProperty,
      { boxWidth: 16 }
    );

This works in the sense that the TS compiler is okay with it, but it seems to me like we're working around the type system rather than working with it.

samreid commented 2 years ago

This proposal introduces LinkableElement and demonstrates how it can address cases like the one the preceding comment:

```diff Index: main/tambo/js/demo/testing/view/AmplitudeModulatorDemoNode.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/tambo/js/demo/testing/view/AmplitudeModulatorDemoNode.ts b/main/tambo/js/demo/testing/view/AmplitudeModulatorDemoNode.ts --- a/main/tambo/js/demo/testing/view/AmplitudeModulatorDemoNode.ts (revision 86d083027e0c0e507152e5187e56816ad135d560) +++ b/main/tambo/js/demo/testing/view/AmplitudeModulatorDemoNode.ts (date 1648501658978) @@ -65,7 +65,7 @@ // LFO enabled control const lfoEnabled = new Checkbox( new Text( 'LFO Enabled', { font: LABEL_FONT } ), - amplitudeModulatorDemo.amplitudeModulator.enabledProperty as BooleanProperty, + amplitudeModulatorDemo.amplitudeModulator.enabledProperty, { boxWidth: 16 } ); Index: main/sun/js/Checkbox.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/sun/js/Checkbox.ts b/main/sun/js/Checkbox.ts --- a/main/sun/js/Checkbox.ts (revision 22a00bb2b17ccb4bc8ce9882a097e8b587ab6475) +++ b/main/sun/js/Checkbox.ts (date 1648501674263) @@ -23,6 +23,8 @@ import sun from './sun.js'; import ISoundPlayer from '../../tambo/js/ISoundPlayer.js'; import Utterance, { IAlertable } from '../../utterance-queue/js/Utterance.js'; +import IProperty from '../../axon/js/IProperty.js'; +import LinkableElement from '../../tandem/js/PhetioObject.js'; // constants const BOOLEAN_VALIDATOR = { valueType: 'boolean' }; @@ -69,7 +71,7 @@ * @param providedOptions * @mixes {Voicing} */ - constructor( content: Node, property: Property, providedOptions?: CheckboxOptions ) { + constructor( content: Node, property: IProperty & LinkableElement, providedOptions?: CheckboxOptions ) { const options = optionize( { Index: main/tandem/js/PhetioObject.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/tandem/js/PhetioObject.ts b/main/tandem/js/PhetioObject.ts --- a/main/tandem/js/PhetioObject.ts (revision 264f9ebf1c92925159221614c509edbb1aea182e) +++ b/main/tandem/js/PhetioObject.ts (date 1648501807831) @@ -118,6 +118,8 @@ phetioDesigned?: boolean; }; +export type LinkableElement = Pick; + class PhetioObject { // assigned in initializePhetioObject - see docs at DEFAULTS declaration @@ -552,7 +554,7 @@ * - otherwise it gracefully opts out * @param [options] */ - addLinkedElement( element: PhetioObject, options?: any ): void { + addLinkedElement( element: LinkableElement, options?: any ): void { if ( !this.isPhetioInstrumented() ) { // set this to null so that you can't addLinkedElement on an uninitialized PhetioObject and then instrument @@ -561,8 +563,6 @@ return; } - assert && assert( element instanceof PhetioObject, 'element must be of type PhetioObject' ); - // In some cases, UI components need to be wired up to a private (internal) Property which should neither be // instrumented nor linked. if ( PHET_IO_ENABLED && element.isPhetioInstrumented() ) { @@ -701,16 +701,14 @@ * @private */ class LinkedElement extends PhetioObject { - element: PhetioObject; + element: LinkableElement; /** * @param {PhetioObject} coreElement * @param {Object} [options] */ - constructor( coreElement: PhetioObject, options?: any ) { + constructor( coreElement: LinkableElement, options?: any ) { assert && assert( !!coreElement, 'coreElement should be defined' ); - assert && assert( coreElement instanceof PhetioObject, 'coreElement should be PhetioObject' ); - assert && assert( coreElement.tandem, 'coreElement should have a tandem' ); options = merge( { phetioType: LinkedElementIO Index: main/axon/js/EnabledComponent.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/axon/js/EnabledComponent.ts b/main/axon/js/EnabledComponent.ts --- a/main/axon/js/EnabledComponent.ts (revision 1e047b968c6d6d1ff9740e2d98f7f68868fb97ec) +++ b/main/axon/js/EnabledComponent.ts (date 1648501724470) @@ -15,6 +15,7 @@ import Tandem from '../../tandem/js/Tandem.js'; import axon from './axon.js'; import IProperty from './IProperty.js'; +import LinkableElement from '../../tandem/js/PhetioObject.js'; // constants const DEFAULT_OPTIONS = { @@ -27,7 +28,7 @@ export type EnabledComponentOptions = { // if not provided, a Property will be created - enabledProperty?: IProperty | null; + enabledProperty?: IProperty & LinkableElement | null; // initial value of enabledProperty if we create it, ignored if enabledProperty is provided enabled?: boolean; @@ -45,7 +46,7 @@ export default class EnabledComponent { - enabledProperty: IProperty; + enabledProperty: IProperty & LinkableElement; private disposeEnabledComponent: () => void; ```

pixelzoom commented 2 years ago

Re https://github.com/phetsims/axon/issues/382#issuecomment-1081154975 ...

I'm not excited about having to do IProperty<boolean> & LinkableElement wherever some part of the implementation (often in a superclass, far from where I'm adding LinkableElement) needs to call addLinkedElement. But maybe that's what we're stuck with, give the Property/TinyProperty "problem".
When we discover some other aspect of PhetioObject that's needed for IProperty/IReadonlyProperty, will we add yet another type that we'll need to patch in like LinkableElement?

samreid commented 2 years ago

Thanks, I agree fully. Here is a better proposal:

```diff Index: main/axon/js/TinyProperty.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/axon/js/TinyProperty.ts b/main/axon/js/TinyProperty.ts --- a/main/axon/js/TinyProperty.ts (revision 1e047b968c6d6d1ff9740e2d98f7f68868fb97ec) +++ b/main/axon/js/TinyProperty.ts (date 1648503718410) @@ -14,6 +14,7 @@ import TinyEmitter from './TinyEmitter.js'; import IProperty from './IProperty.js'; import { PropertyLinkListener, PropertyLazyLinkListener, PropertyListener } from './IReadOnlyProperty.js'; +import LinkableElement from '../../tandem/js/PhetioObject.js'; type ComparableObject = { equals: ( a: any ) => boolean; @@ -168,7 +169,7 @@ * * NOTE: Duplicated with Property.linkAttribute */ - linkAttribute( object: { [ key in Attr ]: T }, attributeName: Attr ) { // eslint-disable-line + linkAttribute( object: { [key in Attr]: T }, attributeName: Attr ) { // eslint-disable-line const handle = ( value: T ) => { object[ attributeName ] = value; }; this.link( handle ); return handle; @@ -188,6 +189,13 @@ isPhetioInstrumented(): boolean { return false; } + + /** + * This is to build out the "Property-like" interface for usages that can take a TinyProperty or Property interchangeably + */ + get phetioFeatured(): boolean { + return false; + } /** * Returns true if the value can be set externally, using .value= or set() Index: main/tambo/js/demo/testing/view/AmplitudeModulatorDemoNode.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/tambo/js/demo/testing/view/AmplitudeModulatorDemoNode.ts b/main/tambo/js/demo/testing/view/AmplitudeModulatorDemoNode.ts --- a/main/tambo/js/demo/testing/view/AmplitudeModulatorDemoNode.ts (revision 782eb89fb23646b14791c3bdb08558b9168ae134) +++ b/main/tambo/js/demo/testing/view/AmplitudeModulatorDemoNode.ts (date 1648503684661) @@ -65,7 +65,7 @@ // LFO enabled control const lfoEnabled = new Checkbox( new Text( 'LFO Enabled', { font: LABEL_FONT } ), - amplitudeModulatorDemo.amplitudeModulator.enabledProperty as BooleanProperty, + amplitudeModulatorDemo.amplitudeModulator.enabledProperty, { boxWidth: 16 } ); Index: main/sun/js/Checkbox.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/sun/js/Checkbox.ts b/main/sun/js/Checkbox.ts --- a/main/sun/js/Checkbox.ts (revision 22a00bb2b17ccb4bc8ce9882a097e8b587ab6475) +++ b/main/sun/js/Checkbox.ts (date 1648503684657) @@ -23,6 +23,7 @@ import sun from './sun.js'; import ISoundPlayer from '../../tambo/js/ISoundPlayer.js'; import Utterance, { IAlertable } from '../../utterance-queue/js/Utterance.js'; +import IProperty from '../../axon/js/IProperty.js'; // constants const BOOLEAN_VALIDATOR = { valueType: 'boolean' }; @@ -69,7 +70,7 @@ * @param providedOptions * @mixes {Voicing} */ - constructor( content: Node, property: Property, providedOptions?: CheckboxOptions ) { + constructor( content: Node, property: IProperty, providedOptions?: CheckboxOptions ) { const options = optionize( { Index: main/tandem/js/PhetioObject.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/tandem/js/PhetioObject.ts b/main/tandem/js/PhetioObject.ts --- a/main/tandem/js/PhetioObject.ts (revision 264f9ebf1c92925159221614c509edbb1aea182e) +++ b/main/tandem/js/PhetioObject.ts (date 1648503291766) @@ -118,6 +118,8 @@ phetioDesigned?: boolean; }; +export type LinkableElement = Pick; + class PhetioObject { // assigned in initializePhetioObject - see docs at DEFAULTS declaration @@ -552,7 +554,7 @@ * - otherwise it gracefully opts out * @param [options] */ - addLinkedElement( element: PhetioObject, options?: any ): void { + addLinkedElement( element: LinkableElement, options?: any ): void { if ( !this.isPhetioInstrumented() ) { // set this to null so that you can't addLinkedElement on an uninitialized PhetioObject and then instrument @@ -561,8 +563,6 @@ return; } - assert && assert( element instanceof PhetioObject, 'element must be of type PhetioObject' ); - // In some cases, UI components need to be wired up to a private (internal) Property which should neither be // instrumented nor linked. if ( PHET_IO_ENABLED && element.isPhetioInstrumented() ) { @@ -701,16 +701,14 @@ * @private */ class LinkedElement extends PhetioObject { - element: PhetioObject; + element: LinkableElement; /** * @param {PhetioObject} coreElement * @param {Object} [options] */ - constructor( coreElement: PhetioObject, options?: any ) { + constructor( coreElement: LinkableElement, options?: any ) { assert && assert( !!coreElement, 'coreElement should be defined' ); - assert && assert( coreElement instanceof PhetioObject, 'coreElement should be PhetioObject' ); - assert && assert( coreElement.tandem, 'coreElement should have a tandem' ); options = merge( { phetioType: LinkedElementIO Index: main/axon/js/IReadOnlyProperty.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/axon/js/IReadOnlyProperty.ts b/main/axon/js/IReadOnlyProperty.ts --- a/main/axon/js/IReadOnlyProperty.ts (revision 1e047b968c6d6d1ff9740e2d98f7f68868fb97ec) +++ b/main/axon/js/IReadOnlyProperty.ts (date 1648503623376) @@ -23,6 +23,7 @@ unlinkAttribute( listener: PropertyLinkListener ): void; hasListener( listener: PropertyLinkListener ): boolean; isPhetioInstrumented(): boolean; + get phetioFeatured(): boolean; isSettable(): boolean; dispose(): void; ```

This adds the LinkedElement API to IReadOnlyProperty, and TinyProperty implements get phetioFeatured as a constant false, like it already does for isPhetioInstrumented.

pixelzoom commented 2 years ago

I applied the above patch and reviewed. That looks like a readable way to address the needs of addLinkedElement.

samreid commented 2 years ago

I applied the patch from above. Not sure if this issue should be closed or left open for further discussion or PSA.

pixelzoom commented 2 years ago

We probably need an issue (or issues?) to go back and change all the places where we were previously forced to use Property, and use IProperty or IReadOnlyProperty as appropriate.

For example, I'm going to clean up Geometric Optics in this issue: https://github.com/phetsims/geometric-optics/issues/416. But there are definitely cases in sun/scenery-phet/joist that need to be revised.

pixelzoom commented 2 years ago

In the above commit, I attempted to replace Property with IProperty where appropriate in sun and scenery-phet APIs.

pixelzoom commented 2 years ago

@samreid There's still something missing from the LinkableElement API -- access to tandem.

General example:

class MyClass extends PhetioObject {
  constructor( myProperty: IProperty<boolean>, ... ) {
    ...
    this.addLinkedElement( myProperty, {
      tandem: options.tandem.createTandem( myProperty.tandem.name )
    } );
  }
}

myProperty if flagged with TS2339: Property 'tandem' does not exist on type 'IProperty<boolean>'.

Getting the tandem name from the element is a very common pattern that I've used everywhere. It ensures that the tandem name used for the link is the same as the element. And it's essential when there are multiple instances of MyClass, with diffferent tandem names for myProperty.

samreid commented 2 years ago

Thanks, I committed a proposed fix. Can you please review?

pixelzoom commented 2 years ago

Looks good, thanks.

I think the only thing left to do here is https://github.com/phetsims/axon/issues/382#issuecomment-1086037626.

samreid commented 2 years ago

Thanks, it seems the next step for https://github.com/phetsims/axon/issues/382#issuecomment-1086037626 is discussion in dev meeting.

pixelzoom commented 2 years ago

... and go back and correct places where IProperty or IReadOnlyProperty should have been used?

zepumph commented 2 years ago

We discussed this as a whole dev team today. We are having trouble as the Property interface intersects with PhET-iO.

On one hand, if we have a Checkbox, PhET devs should pass a Property in there to decrease technical debt when we convert to PhET-iO (because it will have to be an instrumented Property). But that cascades all the way through to the model, and isn't necessarily that helpful to exclude TinyProperty at every stage of things. Also for any dev using Checkbox (an open source component of an open source library) who doesn't care about PhET-iO, TinyProperty should work just fine.

We will discuss this further next week.

samreid commented 2 years ago

Today we agreed to remove

  // isPhetioInstrumented(): boolean;
  // get phetioFeatured(): boolean;
  // get tandem(): Tandem;

from IReadOnlyProperty and to rename AbstractProperty => ReadOnlyProperty. Sun components will use Property instead of IProperty. We will use instanceof checks where necessary in Node, etc. I'll take the lead.

samreid commented 2 years ago

I made it very far on the recommendations from today, but ran into roadblocks from DynamicProperty. Rather than abandon the work, I decided to commit and add ts-ignores. @jonathanolson can you please search the codebase for https://github.com/phetsims/axon/issues/382 to see the remaining problems, two of which are in density?

samreid commented 2 years ago

In https://github.com/phetsims/density-buoyancy-common/issues/74 we established LinkableProperty which will work well here.

samreid commented 2 years ago

I was able to address the remaining ts-ignore. I think this issue can be closed.

phetsims / axon

addLinkedElement is causing APIs to use Property instead of IProperty/IReadOnlyProperty #382