IReadOnlyProperty as an option doesn't support setting value on that Property

[samreid]

Like https://github.com/phetsims/build-a-nucleus/issues/13 but for all repos

[samreid]

I used this bad-text rule to find many occurrences:

 ': DerivedProperty<', // see https://github.com/phetsims/build-a-nucleus/issues/13

However, changing DerivedProperty.and and DerivedProperty.not yields type errors in PlayPauseStepButtonGroup.ts related to EnabledComponent, which requires IProperty. @zepumph can you please comment how we should deal with this in EnabledComponent?

[zepumph]

@pixelzoom, @samreid and I found that Node.mutate was greatly facilitating a lack of type safety for allowing derivedProperties to be set into fields that were IProperty. Basically we have:

• visibleProperty option is a ReadonlyProperty
• setVisibleProperty takes an IProperty

So you could never call myNode.setVisibleProperty( myDerivedProperty ), but mutate allows us to bypass this via this line:

https://github.com/phetsims/scenery/blob/6ecc0efd397bec0200f5fa296d3d11f65b666373/js/nodes/Node.ts#L6341-L6342

In https://github.com/phetsims/utterance-queue/issues/86 we did the same kind of punt in EnabledComponent so that it can support an enabledProperty that is potentially settable or not. Note that setEnabled asserts that we are settable, but this isn't possible when getting the enabledProperty directly.

https://github.com/phetsims/axon/blob/4afa6aa62d5aec75f2661b0611518ba1f410fa11/js/EnabledComponent.ts#L63-L65

@jonathanolson, you were not part of this conversation, would you mind reviewing this to see if you have any other thoughts about this.

[jonathanolson]

It seems like setVisibleProperty should take a ReadonlyProperty then, and that fixes this case, no?

[zepumph]

But that isn't correct. You are allowed to set a property passed to a Node, so it still isn't correct.

[zepumph]

Oops forgot to reassign.

[jonathanolson]

Similar to https://github.com/phetsims/scenery/issues/1446

[jonathanolson]

Pending on https://github.com/phetsims/scenery/issues/1446, I believe it might be good to just set the setters all as taking read-only Properties, and we'll have runtime failures if those happen. @zepumph thoughts?

[zepumph]

That is fine with me! What about the getters though? Do we want to support settable as return values still?

[jonathanolson]

What about the getters though? Do we want to support settable as return values still?

We would have getters return the "settable" type presumably.

[zepumph]

Waiting for feedback over in https://github.com/phetsims/scenery/issues/1446 to continue here.

[zepumph]

All is good over in https://github.com/phetsims/scenery/issues/1446, I'll update here too.

[zepumph]

Excellent! Thanks @jonathanolson. You can pass DerivedProperties into Node and Text property setters.