Closed zepumph closed 1 year ago
There are 23 spots (as well as all future spots) where we would have to handle this in sim-specific usages instead of generally. I would say this is not correct or a step in the right direction.
@samreid please continue if you would like, but most likely this should have a conversation with CM before committing. Feel free to close.
I ~feel~ suspect this is the appropriate way to go, and the price of type checks at each site is worth it. But I also agree we should discuss it with @pixelzoom before proceeding.
@pixelzoom the main proposal is to change PhetioObject.addLinkedElement from:
public addLinkedElement( element: LinkableElement, providedOptions?: LinkedElementOptions ): void {
to
public addLinkedElement( element: PhetioObject, providedOptions?: LinkedElementOptions ): void {
recall LinkableElement
is defined as type LinkableElement = Pick<PhetioObject, 'phetioFeatured' | 'isPhetioInstrumented'>;
This means that call sites would need to guard like so:
if ( linkedProperty && linkedProperty instanceof PhetioObject ) {
this.addLinkedElement( linkedProperty, {
tandem: options.tandem.createTandem( 'valueProperty' )
} );
}
There are more examples of this in @zepumph patch. @pixelzoom can you please review this issue and advise?
The API change to addLinkedElement
sounds reasonable. And that would be consistent with removeLinkedElement
, which currently requires a PhetioObject:
public removeLinkedElements( potentiallyLinkedElement: PhetioObject )
But is it going to be a problem not to have a link back to the core element? That one of the things that LinkedElement provides:
class LinkedElement extends PhetioObject {
public readonly element: LinkableElement;
Here's a patch to PhetioObject that makes addLinkedElement require a PhetioObject:
Results of tsc on the entire code base:
Found 24 errors in 16 files.
Errors Files 2 ../../../beers-law-lab/js/beerslaw/model/BeersLawSolution.ts:120 1 ../../../beers-law-lab/js/common/model/Solute.ts:118 1 ../../../calculus-grapher/js/common/view/LabeledLinesNode.ts:53 1 ../../../calculus-grapher/js/common/view/LabeledPointsNode.ts:45 1 ../../../equality-explorer/js/common/model/Variable.ts:66 1 ../../../equality-explorer/js/solveit/model/SolveItLevel.ts:138 1 ../../../joist/js/Screen.ts:193 1 ../../../ph-scale/js/common/model/Solute.ts:105 6 ../../../ph-scale/js/common/view/graph/GraphNode.ts:176 3 ../../../ph-scale/js/common/view/ParticleCountsNode.ts:125 1 ../../../ph-scale/js/micro/view/MicroPHAccordionBox.ts:51 1 ../../../scenery-phet/js/ZoomButtonGroup.ts:132 1 ../../../sun/js/Checkbox.ts:228 1 ../../../sun/js/ComboBox.ts:420 1 ../../../sun/js/Slider.ts:503 1 ../../../tandem/js/PhetioObject.ts:616
Yes, with this proposal we would also change:
class LinkedElement extends PhetioObject {
public readonly element: LinkableElement;
to
class LinkedElement extends PhetioObject {
public readonly element: PhetioObject;
However the patch in the preceding comment incorrectly changes private linkedElements!: LinkedElement[] | null;
to private linkedElements!: PhetioObject[] | null;
. Perhaps due to a confusion between LinkedElement
vs LinkableElement
?
Ah, sorry -- indeed, my patch changed a few internal things incorrectly. But the patch was intended mainly to determine how many errors would occur by changing the signature of addLinkedElement
. And (after correcting my patch) external to PhetioObject, I still see 23 errors in the files listed below. I looked through the first few files, and they would take some time to address. You might want to inspect them too before proceeding.
2 ../../../beers-law-lab/js/beerslaw/model/BeersLawSolution.ts:120 1 ../../../beers-law-lab/js/common/model/Solute.ts:118 1 ../../../calculus-grapher/js/common/view/LabeledLinesNode.ts:53 1 ../../../calculus-grapher/js/common/view/LabeledPointsNode.ts:45 1 ../../../equality-explorer/js/common/model/Variable.ts:66 1 ../../../equality-explorer/js/solveit/model/SolveItLevel.ts:138 1 ../../../joist/js/Screen.ts:193 1 ../../../ph-scale/js/common/model/Solute.ts:105 6 ../../../ph-scale/js/common/view/graph/GraphNode.ts:176 3 ../../../ph-scale/js/common/view/ParticleCountsNode.ts:125 1 ../../../ph-scale/js/micro/view/MicroPHAccordionBox.ts:51 1 ../../../scenery-phet/js/ZoomButtonGroup.ts:132 1 ../../../sun/js/Checkbox.ts:228 1 ../../../sun/js/ComboBox.ts:420 1 ../../../sun/js/Slider.ts:503
@pixelzoom, I am not worried about the 23 usages. As I reported in https://github.com/phetsims/tandem/issues/299#issuecomment-1588075815, instead I am interested in how we feel about this pattern generally. Would it be acceptable to use an instanceof
check before calls to addLinkedElement
in your code when the types don't align directly into the ReadOnlyProperty
hierarchy (or elsewhere).
I'm would be a bit surprised if you preferred this direction. The reason why we are here is because of a single instanceof
assertion/type cast in phet-io internal code, and the result is a propagation of 23+ instanceof
checks in common code. I would expect we would prefer a graceful API where addLinkedElement
can be added to any interface as a no-op to support a simpler common code library. I can't say I care too much either way though.
Would it be acceptable to use an instanceof check before calls to
addLinkedElement
in your code when the types don't align directly into theReadOnlyProperty
hierarchy (or elsewhere).
My first reaction is "that sounds awful". My second reaction is "show me an example before I give you my first reaction" :)
The patch in https://github.com/phetsims/tandem/issues/299#issuecomment-1588075815 has multiple examples, sorry if that wasn't clear.
Example for ComboBox:
- this.addLinkedElement( property, {
+ property instanceof PhetioObject && this.addLinkedElement( property, {
tandem: options.tandem.createTandem( 'property' )
} );
No, I don't like that direction.
@zepumph how about changing the signature of addLinkedElement like this:
If we go with that, how would we change usages like in Slider.ts?
@samreid and I really like the above patch. It moves the burden onto PhetioObject.addLinkedElement(), and allows us to completely get rid of the Linkable*
types that we created in https://github.com/phetsims/axon/issues/382. In my opinion those are clunky and not actually solving any problems. We would like to talk @pixelzoom before continuing though. I'll reach out as priority permits.
I put something on the books for monday.
@samreid @pixelzoom and I all discussed this, and we were really going back and forth about where the burden should be about phet-io support in common code. We landed on a smaller change, since LinkableProperty is central the UI common code, and ensures type safe dependence on PhetioObject. We solidified that a bit more, making it clear that you need to pass a ReadOnlyProperty
(PhetioObject
) to support linked elements. A typescript-safe approach is best here to help out phet-io developers, and likely there are few cases where open source/non-phetsims usages of sun and scenery-phet would yield problems (like when using TinyProperty).
Commit coming soon.
Bringing the summary in https://github.com/phetsims/tandem/issues/299#issuecomment-1599200308 to the bottom of the issue:
@samreid @pixelzoom and I all discussed this, and we were really going back and forth about where the burden should be about phet-io support in common code. We landed on a smaller change, since LinkableProperty is central the UI common code, and ensures type safe dependence on PhetioObject. We solidified that a bit more, making it clear that you need to pass a ReadOnlyProperty (PhetioObject) to support linked elements. A typescript-safe approach is best here to help out phet-io developers, and likely there are few cases where open source/non-phetsims usages of sun and scenery-phet would yield problems (like when using TinyProperty).
Now there are only 19 files using PhetioProperty. This is a big improvement from LinkableProperty, and is only needed where non Property
instances needed a settable property interface AND phet-io support at the same time (like dynamic properties). We also kept it in our UI components for that same flexibility.
@samreid should we do any more work to try to limit the current usages of PhetioProperty? I also updated the doc a bit more. Can you please spot check.
I reviewed usages of PhetioProperty and was able to remove one. The rest seem appropriate. I don't feel the name is perfect, but I can't think of a better name. I feel this issue is ready to close.
We support LinkableElement for our common UI code, but we should be type safe in calling addLinkedElement, and only do that for PhetioObjects. @samreid and I were working on a patch and made it work well in initial testing, we will take a look.