Why does NumberProperty with `range: null` have instrumented `rangeProperty`?

[pixelzoom]

Related to instrumentation of equality-explorer https://github.com/phetsims/equality-explorer/issues/192 and other PhET-iO sims ...

I often have a NumberProperty that has no range. And in those cases, I get a default rangeProperty that is instrumented, has a null value, and is phetioReadOnly: true.

For example:

Questions:

(1) Was this a conscious design decision? I suspect not. I suspect that this is a bug, and that the default rangeProperty should not be instrumented if NumberPropertyOptions.range is null.

(2) If this is not a bug and was a design decision... Is there an option to not instrument the default rangeProperty? If so, when should I be using it?

[pixelzoom]

Maybe this is a complicated way of telling the user that "this Property has no range". If that's the case, I think it would be preferrable to simply not instrument rangeProperty. And if a NumberProperty has no rangeProperty, then it should be clear that it has no range.

[samreid]

@zepumph, you have more experience in this area, can you please advise?

[arouinfar]

Was this a conscious design decision?

No, not that I'm aware of. I've asked for a rangeProperty to be a particular range, but I've never explicitly asked for a null range.

I vaguely remember @zepumph mentioning that rangeProperty had been over-instrumented at some point and it was being scaled back. (But I could be wrong.)

[samreid]

@zepumph said if the default value for range is null, skipping the rangeProperty instrumentation sounds good.

[pixelzoom]

I've been working around this in equality-explorer Variable.ts like this:

this.valueProperty = new NumberProperty( options.value, {
  numberType: 'Integer',
  range: options.range,
...
  // Do not instrument rangeProperty when there is no range. See https://github.com/phetsims/axon/issues/423
  rangePropertyOptions: ( options.range === null ) ? { tandem: Tandem.OPT_OUT } : {}

[samreid]

I committed a proposed solution, @zepumph or @pixelzoom does that look correct to you? If so, @pixelzoom would you like to visit your workarounds like https://github.com/phetsims/axon/issues/423#issuecomment-1299371146 to remove them?

[zepumph]

Looks great Thanks.

[pixelzoom]

I see a problem. options.rangePropertyOptions should always be applied. But it is now (silently!) ignored if options.range is null. This makes it impossible to optionally instrument rangeProperty when it's initial value is null, and I intend to set its value to non-null later.

For example rangePropertyOptions is ignored in this example, and I end up with an uninstrumented rangeProperty:

// Create a NumberProperty whose range is instrumented, but is initially null and will be set later, because
// it needs to be computed, or may be dynamic.
const numberProperty = new NumberProperty( 0, {
  range: null,
  rangePropertyOptions: {
    tandem: options.tandem.createTandem( 'numberProperty' )
  }
} );
...
// Set the desired range for numberProperty.
numberProperty.rangeProperty.value = new Range( min, max );

So maybe this change?

const rangePropertyDefaults = options.range ? {
       phetioDocumentation: 'provides the range of possible values for the parent NumberProperty',
       phetioValueType: NullableIO( Range.RangeIO ),
       phetioReadOnly: true,
       tandem: options.tandem.createTandem( RANGE_PROPERTY_TANDEM_NAME )
     }, options.rangePropertyOptions );
-   } : {};
+   } : options.rangePropertyOptions;

But... This may also have been the intention of instrumenting rangeProperty regardless of its initial value. While null is the initial value, it may not be the only value, and callers who do not intend t change to a non-null range can opt out via rangePropertyOptions: { tandem: Tandem.OPT_OUT }. So should what is a better default behavior when range: null, opt-in or opt-out? @samreid @zepumph thoughts?

[pixelzoom]

From my original comment:

(2) If this is not a bug and was a design decision... Is there an option to not instrument the default rangeProperty? If so, when should I be using it?

So perhaps the correct answer is: Yes, this was a design decision, but not sufficiently documented. And if you have a range that will always be null, then you should opt-out with rangePropertyOptions: { tandem: Tandem.OPT_OUT }.

[pixelzoom]

@samreid @zepumph let's hop on a quick Zoom call to decide what to do here. It will save some back-and-forth.

[zepumph]

I totally agree with you. @pixelzoom, ignoring some of the Tandem.VALIDATION improvements I snuck into this commit, do you like this sentiment better?

[pixelzoom]

Very nice, thanks.

I've removed the workaround for equality-explorer in the commit below.

Closing.