NumberProperty Typescript improvements around rangeProperty

[zepumph]

@jonathanolson @marlitas and @samreid and I updated NumberProperty's range pattern to get rid of the notion of null. It is working really well, and we are happy to move in this direction. Instead of a default of null, we default to Range.EVERYTHING (same pattern as for bounds).

We are happy to get rid of asRanged() usages for typescript handling of NumberProperty.rangeProperty.

From here we may look into a default Range that throws errors if you try to mutate it:

class InfiniteRangeType extends Range {

  public override set min( min: number ) {
    throw new Error( 'unsettable' );
  }

  public override setMin( min: number ): void {
    throw new Error( 'unsettable' );
  }

  public override set max( max: number ) {
    throw new Error( 'unsettable' );
  }

  public override setMax( max: number ): void {
    throw new Error( 'unsettable' );
  }

  public override setMinMax( min: number, max: number ): this {
    throw new Error( 'unsettable' );
  }
}

export const InfiniteRange = new InfiniteRangeType( Number.NEGATIVE_INFINITY, Number.POSITIVE_INFINITY );

dot.register( 'Range', Range );

I'll check back in on Monday to see if there was trouble from the commit.

[pixelzoom]

It looks like after changes by @zepumph, NumberProperty always has non-null rangeProperty and range. Is that correct? If so...

Questions:

• I see that NumberProperty asRanged was deleted. Why does type RangedProperty still exist? It was created (with asRanged) to address the problem of a NumberProperty with no range. Should it be deleted too? I looks like some usages were replaced, but I still see RangedProperty imported in 5 files.

• It looks like uses of non-null assertion operator like numberProperty.range! and numberProperty.rangeProperty! were cleaned up. Is someone also going to address redundant assertions like assert( numberProperty.rangeProperty ) and assert( numberProperty.range )? That's how I found out about this change -- by discovering a redundant assertion in ElectronegativitySlider.ts:

    const range = atom.electronegativityProperty.range;
    assert && assert( range );

• Was there a PSA about this? Was there an announcement on the Google Group? It's a significant API change, and caught me off guard. And it's a breaking change for anyone who was using NumberPropertyOptions range: null.

[pixelzoom]

In the above commits, I deleted 21 redundant range assertions that I was able to identify in my code. There are probably others that I haven't identified, and I didn't touch code that I didn't write.

[zepumph]

• I see that NumberProperty asRanged was deleted. Why does type RangedProperty still exist? It was created (with asRanged) to address the problem of a NumberProperty with no range. Should it be deleted too? I looks like some usages were replaced, but I still see RangedProperty imported in 5 files.

Yes, this interface should stick around to signify an interface that has a range. It is used in the project by DynamicProperties that can't be a NumberProperty. I updated NumberProperty to say that it implements RangedProperty though, for clarity.

• It looks like uses of non-null assertion operator like numberProperty.range! and numberProperty.rangeProperty! were cleaned up. Is someone also going to address redundant assertions like assert( numberProperty.rangeProperty ) and assert( numberProperty.range )? That's how I found out about this change -- by discovering a redundant assertion in ElectronegativitySlider.ts:

I found a couple more assertions with assert\( \w+.range[ ,] that I removed, but I only did this in Typescript when I could ensure that the Type was a NumberProperty specifically.

• Was there a PSA about this? Was there an announcement on the Google Group? It's a significant API change, and caught me off guard. And it's a breaking change for anyone who was using NumberPropertyOptions range: null.

Good call! I just posted to the google group:

In https://github.com/phetsims/axon/issues/425, NumberProperty's range option was changed to no longer accept null as a value. Now all NumberProperty instances have a defined range, defaulting to (-∞,∞). Thus NumberProperty.prototype.range and NumberProperty.prototype.rangeProperty.value will always be of type Range.

Thanks for the impromptu review. Anything else here?

[pixelzoom]

Yes, this interface should stick around to signify an interface that has a range. It is used in the project by DynamicProperties that can't be a NumberProperty. I updated NumberProperty to say that it implements RangedProperty though, for clarity.

Two remaining issues:

(1) Factor out RangedProperty.ts. type RangedProperty is not specific to NumberProperty, and it's used in other places that have nothing to do with NumberProperty. So what is it defined in NumberProperty.ts? I also see UnitConversionProperty using NumberProperty.DEFAULT_RANGE, which seems like a very odd coupling. Should we factor out type RangedProperty and DEFAULT_RANGE to RangedProperty.ts?

(2) Address hackery in UnitConversionProperty.

You said:

It is used in the project by DynamicProperties that can't be a NumberProperty

I see one such DynamicProperty: UnitConversionProperty. It contains this hackery:

public constructor( property: TProperty<number>, ... ) 
...
    if ( ( property as NumberProperty ).rangeProperty ) {

Why isn't the constructor parameter property: RangedProperty?

[zepumph]

Why isn't the constructor parameter property: RangedProperty?

The type was written to support provided Properties with or without Range, but I updated the code to be explicit and type safe instead of type casting.

I also renamed to TRangedProperty because @marlitas and I thought that was a bit less confusing in a conversation yesterday over the Property hierarchy+TypeScript.

Do you think that LinkableProperty should be TLinkableProperty?

Anything else here @pixelzoom?

[pixelzoom]

Do you think that LinkableProperty should be TLinkableProperty?

No, I don't think that's necessary.

Anything else here @pixelzoom?

Nope, closing. Thanks!