TReadOnlyProperty is not linkable

[pixelzoom]

Having to add & LinkableElement in cases like this seems unfortunate:

public readonly massProperty: TReadOnlyProperty<number> & LinkableElement;
...
this.massProperty = new DerivedProperty( ... );

It seems like any "read-only Property" should be linkable, since writeability is not required for something to be a PhET-iO linked element. (It is required to be a PhET-iO element.)

I've had to do this in 9 places during pH Scale conversion to TypeScript https://github.com/phetsims/ph-scale/issues/242.

Probably low priority for now. @samreid thoughts?

[samreid]

We currently have axon/js/LinkableProperty.ts:

type LinkableProperty<T> = TProperty<T> & LinkableElement;
export default LinkableProperty;

Should we add LinkableReadOnlyProperty? Some places like Checkbox require a writable linkable property.

[pixelzoom]

Should we add LinkableReadOnlyProperty? Some places like Checkbox require a writable linkable property.

Probably yes. I still don't have a good picture of the "Property" class/type hierachy in my head. It's much too complicated. See https://github.com/phetsims/axon/issues/404

[pixelzoom]

I just discovered that Properties in {repo}String.ts are of type TReadOnlyProperty<string>. So I can't create a linked element for any translated string Property. This is a problem in ph-scale, where you should be able to inspect the static Solute instances under ph-scale.global.model.solutes to find out how to change their name.

So Strings.ts needs to either use type TReadOnlyProperty<string> & LinkableElement (yuck), or we need something like LinkableReadOnlyProperty.

[samreid]

I believe we can change the types in {repo}String.ts from TReadOnlyProperty<string> to LinkableProperty<string>. They are already DynamicProperty<string> which already implements LinkableProperty<string>. @jonathanolson or @zepumph sound good to you?

[zepumph]

Yes sounds great to me. I was wondering how far we were going to get using the type instead of the actual Property hierarchy. There is no reason (in my opinion) to keep these as readonly, and instead we should rely on a keen eye from the dev team while using these in sims to narrow the type to readonly as needed.

[pixelzoom]

I added a TODO linked to this issue in ph-scale Solute.ts.

[samreid]

I was about to change TReadOnlyProperty<string> to LinkedProperty<string> in the simStrings.ts files, but wanted to double check first if we would prefer to just be specific and return DynamicProperty<string, string, string>? LinkableProperty doesn't even know that it is a PhetioObject--maybe that interface is too restrictive. So perhaps DynamicProperty<string,string,string> is best for this case?

[pixelzoom]

I'm not familiar with why localized strings are of type DynamicProperty, and I've always steered clear of using it. So I'm going to recue myself from the decision about what's "best for this case". I just need to be able to create a linked element.

[jonathanolson]

LinkableProperty sounds great.

[pixelzoom]

This is now a requirement for https://github.com/phetsims/ph-scale/issues/246, and is blocking.

[pixelzoom]

Also presumably blocking for https://github.com/phetsims/beers-law-lab/issues/292.

[samreid]

I changed modulify, and I'll start adding commits soon.

[samreid]

Implemented above, @pixelzoom can you please test in your context?

[pixelzoom]

👍🏻 Working great in ph-scale and beers-law-lab. Thanks!

[marlitas]

@samreid, while @jonathanolson and I were starting the dynamic strings layout guide we began to question the potential pitfalls of LinkableProperty being writable. We understand this is necessary for PhET-iO, but are worried about sims using properties from the strings files incorrectly and setting in situations where they should not be setting. Do you share these concerns, or should we move forward with LinkableProperty?

[samreid]

Do we need a LinkableReadOnlyProperty?

[samreid]

I would not allow those strings to be writable in cases where they should not be writable. I'd recommend using & LinkableElement or we can create a type alias for it like type LinkableReadOnlyProperty<T> = TReadOnlyProperty<T> & LinkableElement;. @marlitas and @jonathanolson what do you recommend?

[marlitas]

Creating a type alias seems reasonable to me and like the convention will get more use if it's already specified.

I think this issue is within my wheelhouse, but perhaps checking in with @jonathanolson first to make sure I'm not overlooking something?

[marlitas]

I created the type alias LinkableReadOnlyProperty, and was going to add documentation when I realized that I can't find documentation for LinkableProperty or LinkableElement.

I found this documentation for addLinkedElement that seems to cover my understanding of a LinkableElement, perhaps referencing the type alias there is enough?

When an instrumented PhetioObject is linked with another instrumented PhetioObject, this creates a one-way
association which is rendered in Studio as a "symbolic" link or hyperlink. Many common code UI elements use this
automatically. To keep client sites simple, this has a graceful opt-out mechanism which makes this function a
no-op if either this PhetioObject or the target PhetioObject is not instrumented.

@samreid thoughts?

[samreid]

I improved the docs, moved LinkableElement to a top-level type, and improved usages in several repos. @marlitas can you please review? Close if all is well.

[marlitas]

Reviewed. Commits look good. Thanks for doing that work @samreid.

Should part of the work for this issue be improving usages throughout repos or is a PSA enough?

[samreid]

I searched for occurrences of & LinkableElement and replaced occurrences like https://github.com/phetsims/ph-scale/commit/8f95bcc4dda8d1e969845033ad3859afab7a02c8. So maybe start with a PSA?

[marlitas]

I sent out a PSA in the dev-public channel. Closing as completed.