NumberProperty typescript updates

[zepumph]

This file is not up to our standards. I'll update documentation, options pattern, and whatever else I see while I am here for https://github.com/phetsims/studio/issues/253

[zepumph]

@samreid will you please take a quick look at the commits, and then the file and see if there is anything more to do?

[samreid]

Great commits, thanks! Some more questions about NumberProperty:

• I don't think PhET-iO state should be able to change whether a number is integer vs floating. Can this be changed to private readonly? Same for step.

  // @readonly, but cannot set as such because it is set by PhET-iO state.
  numberType: NumberType;

• For step, should we use questionmark instead of |null? It currently reads:

  // @readonly, but cannot set as such because it is set by PhET-iO state.
  step: number | null;

But should it be changed to read like so? Same with validateNumberAndRangeProperty.

  // @readonly, but cannot set as such because it is set by PhET-iO state.
  step?: number;

• Likewise, in the options, several things are marked as questionmark | null, like step?: number | null;. Should that just be step?:number?

• For this code, should any be changed to number?

  private readonly validateNumberAndRangeProperty: ( ( value: any ) => void ) | undefined;

For this code, @chrisklus and I have recommended separating the statics from the instance variables:

  private readonly disposeNumberProperty: () => void;
  static NumberPropertyIO: IOType;
  private readonly resetNumberProperty: () => void;

• This pattern feels problematic, but I don't know a better way to implement it:

  // Minimal types for ranged/stepped Properties
  export type RangedProperty = Property<number> & { range: Range; readonly rangeProperty: IReadOnlyProperty<Range> };
  export type SteppedProperty = Property<number> & { step: number };
  export type RangedSteppedProperty = RangedProperty & SteppedProperty;

  Maybe it's an alternative to mixins? Also, since you can change the range later on, this doesn't feel very safe. Also, I'm not sure the type guards are being used appropriately. It seems isRangedProperty should return boolean or maybe the name should be changed. And should not cast as RangedProperty in the implementation?

• I think we can remove some more assertions now that types are being checked. I think we discussed we don't need the runtime type checks for legacy usages?

  assert && assert( options.range instanceof Range || options.range instanceof Property || options.range === null,
    `invalid range${options.range}` );
  assert && options.step && assert( typeof options.step === 'number', `invalid step:${options.step}` );
  
  assert && assert( options.rangePropertyOptions instanceof Object, 'rangePropertyOptions should be an Object' );
  assert && assert( options.rangePropertyOptions.tandem === Tandem.OPTIONAL || options.rangePropertyOptions.tandem!.name === 'rangeProperty',
    'if instrumenting default rangeProperty, the tandem name should be "rangeProperty".' );
  
  // client cannot specify superclass options that are controlled by NumberProperty
  assert && assert( !options.valueType, 'NumberProperty sets valueType' );

• Like we discussed yesterday, more validation should be moved into the validator instead of in separate links so it can be leveraged by studio value checking:

    // This puts validation at notification time instead of at value setting time. This is especially helpful as it
    // pertains to Property.prototype.setDeferred(), and setting a range and value together.
    this.validateNumberAndRangeProperty && this.link( value => this.validateNumberProperty() );

Anyways, I just tried to jot down many thoughts for this issue. Not all are equally important and some should probably be moved to side issues. Happy to discuss further.

[samreid]

Please also be aware of the removal of step in https://github.com/phetsims/axon/issues/386

[zepumph]

This pattern feels problematic, but I don't know a better way to implement it:

It is @jonathanolson's code, and at one point I spent ~30 minutes poking around with other ideas and couldn't find anything. It doesn't bother me too much, but it is nice, when possible, to not defer to assumptions about runtime conditions.

Like we discussed yesterday, more validation should be moved into the validator instead of in separate links so it can be leveraged by studio value checking:

Better code has been committed from https://github.com/phetsims/studio/issues/253

Everything else was committed. Anything else here?

[jonathanolson]

Maybe it's an alternative to mixins?

It's more descriptive and on the type-end. Mixins would presumably change the behavior. If we have a DerivedProperty or DynamicProperty, they can be massaged into the correct type.

[samreid]

It seems this issue is ready to close.