TinyStaticProperty override of `set` causes TypeScript errors.

[pixelzoom]

Encountered while working on https://github.com/phetsims/geometric-optics/issues/229.

There's a TS problem that occurs when you use a TinyStaticProperty as a dependency for Multilink, DerivedProperty, etc.

Example:

const textNode = new Text( text, options.textOptions );
Property.multilink( [ textNode.boundsProperty, ... ], ... );

textNode.boundsProperty gets flagged with this error:

Type 'TinyStaticProperty' is not assignable to type 'TinyProperty | Property'.   Type 'TinyStaticProperty' is not assignable to type 'TinyProperty'.     The types returned by 'set(...)' are incompatible between these types.       Type 'void' is not assignable to type 'TinyProperty'.

The problem is that TinyStaticProperty overrides the set method of TinyProperty AND changes its interface - there is no longer a return value. (This is a good example of why it's almost always a bad idea to narrow the API when subclassing.)

This JSDoc change resolves the TS error, but it doesn't really match reality, since there is no return value.

  /**
   * Don't set the value of a TinyStaticProperty!
   * @public
   * @override
   *
   * @param {*} value
+  * @returns {TinyStaticProperty}
   */
  set( value ) {
    throw new Error( 'Cannot set a TinyStaticProperty value' );
  }

Assigning to the authors of TinyStaticProperty, @samreid and @jonathanolson.

[pixelzoom]

I went ahead and added a @returns to TinyStaticProperty set:

 * @returns {TinyStaticProperty} does not really return anything, but needed for type checking by TypeScript

[samreid]

Thanks for the fix. I'm considering that we may also want to eliminate chaining from Property.set. It seems like a misplaced and unused feature. I searched for occurrences of property\.set\(.*\)\. and didn't see any occurrence using the chaining. Aqua is halfway through testing all sims and hasn't hit a case that uses it yet.

[samreid]

Aqua fully passed and didn't identify any cases that were using Property.set chaining. So I'll remove it and send a PSA over slack.

[samreid]

On Slack, I said:

Property.set’s chaining feature was removed in https://github.com/phetsims/axon/issues/362. I tested aqua and did a regex search and couldn’t see any usages of it. But please be aware and keep your eyes peeled in case something was using it.

@pixelzoom can you please review this issue? I removed Property.set chaining and update the TinyStaticProperty documentation accordingly.

[pixelzoom]

Reviewed. 👍🏻 closing.

[pixelzoom]

Reopening. It looks like you missed TinyProperty. Its set method is still chaining:

  /**
   * Sets the value and notifies listeners, unless deferred or disposed. You can also use the es5 getter
   * (property.value) but this means is provided for inner loops or internal code that must be fast. If the value
   * hasn't changed, this is a no-op.
   * @public
   *
   * @param {*} value
   * @returns {TinyProperty} this instance, for chaining.
   */
  set( value ) {
    if ( !this.equalsValue( value ) ) {
      const oldValue = this._value;

      this.setPropertyValue( value );

      this.notifyListeners( oldValue );
    }
    return this;
  }

[samreid]

In this issue @pixelzoom and I discovered that WebStorm and tsc had a difference of opinion about whether passing a TinyProperty in place of a TinyStaticProperty (or vice versa) in geometric-optics/LabelNode.ts was acceptable.

Also, I removed the chaining in TinyProperty and everything is passing aqua. I'll commit.

[samreid]

Fixed and ready for review. Close if all is well and thanks for catching this.

[pixelzoom]

In this issue @pixelzoom and I discovered that WebStorm and tsc had a difference of opinion about whether passing a TinyProperty in place of a TinyStaticProperty (or vice versa) in geometric-optics/LabelNode.ts was acceptable.

WebStorm seems to be correct in this case. Based on structural typing (not class hierarchy) a TinyStaticProperty was structurally different than a TinyProperty because the set method did not have the same API. WebStorm was considering the return type, tsc seemed to be ignoring the return type. I'm not sure how that was happening, since WebStorm was pointed at the same tsc used on the command line (version 4.4.4).

Fixed and ready for review. Close if all is well and thanks for catching this.

👍🏻 closing.