Thoughts about LinkedElement

[zepumph]

From discussion with @pixelzoom and @arouinfar.

The chief complaint is that, since a linked element takes its phetioFeatured from the core element. This means that for certain changes when changing a model Property to be phetioFeatured and then committing the overrides. The overrides will then alter the baseline of linkedElements' phetioFeatured.

• (below) Perhaps omit phetioFeature from linked element baselines?
• LinkedElement should be its own file. #204 LinkedElement should be its own file.
• https://github.com/phetsims/studio/issues/187 custom phetioDocumentation for a linked element isn't displayed in studio
• (https://github.com/phetsims/studio/issues/188) The "cut out" shouldn't be editable in by admin mode
• The 'cut out' in the linked element view doesn't update when you edit the core metadata. (see https://github.com/phetsims/studio/issues/96)

[pixelzoom]

To be clear about why this is high priority... The current state of things requires grunt update to be run when changing metadata in Studio. You can't simply change overrides.js, because you might have change an element that is linked to elsewhere. And designers are not set up to deal with grunt update.

[zepumph]

I think that the easiest solution would be to omit phetioFeatured. Perhaps something like:

Index: js/PhetioObject.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/PhetioObject.js	(revision d382effd3ea79df74285669e9df685adeed4f38c)
+++ js/PhetioObject.js	(date 1586386551251)
@@ -646,6 +646,11 @@
 
     super( options );
 
+    if ( this.phetioBaselineMetadata ) {
+      assert && assert( this.phetioBaselineMetadata.hasOwnProperty( 'phetioFeatured' ) );
+      delete this.phetioBaselineMetadata.phetioFeatured;
+    }
+
     // @public (read-only)
     this.element = coreElement;
   }

[zepumph]

Today the phet-io team found an issue with LinkedElement.getMetadata as it pertains to declarative APIs in https://github.com/phetsims/phet-io/issues/1657. I don't think that they should be deleting phetioFeatured from the list, but instead, now that we no longer have baseline files or grunt update commands, we can likely get the phetioFeatured value from the coreElement again.

• assigning to myself to implement.

[samreid]

I read through #161 (comment) and the mentioned commits and did not see any problems. However, since this issue we have eliminated baseline files and it seems like a good idea to run this past @zepumph to see if that changes how we proceed for this issue. Perhaps the preceding comment is part of that.

Also, reducing priority based on the comment in #161 (comment)

[zepumph]

Most everything has been put into side issues, but I want to discuss #161 (comment) in person with @samreid before calling it a day on this issue.

[zepumph]

@samreid and I think that it will be easiest to add this complexity to the Studio side. There we will make sure that overrides with match for the element and linked element.

1. add back in phetioFeatured to LinkedElement
2. assert that phetioFeatured is the same as the core (once both have overrides applied)
3. Add logic to studio to make sure that metadata updated in one is updated in the other.

[zepumph]

Over in phetsims/axon#408 (comment) we determined that we do NOT want LinkedElements to support API calls forwarding to their core elements.

[samreid]

We are improving the overrides process in https://github.com/phetsims/phet-io/issues/1873. That will eliminate problems listed above like:

This means that for certain changes when changing a model Property to be phetioFeatured and then committing the overrides. The overrides will then alter the baseline of linkedElements' phetioFeatured.

From the above:

add back in phetioFeatured to LinkedElement

That code looks like this:

  public constructor( coreElement: LinkableElement, providedOptions?: LinkedElementOptions ) {
    assert && assert( !!coreElement, 'coreElement should be defined' );

    const options = optionize<LinkedElementOptions, EmptySelfOptions, PhetioObjectOptions>()( {
      phetioType: LinkedElementIO,
      phetioState: true
    }, providedOptions );

    // References cannot be changed by PhET-iO
    assert && assert( !options.hasOwnProperty( 'phetioReadOnly' ), 'phetioReadOnly set by LinkedElement' );
    options.phetioReadOnly = true;

    // By default, this linked element's baseline value is the overridden value of the coreElement. This allows
    // the them to be in sync by default, but also allows the linked element to be overridden in studio.
    assert && assert( !options.hasOwnProperty( 'phetioFeatured' ), 'phetioFeatured set by LinkedElement' );
    options.phetioFeatured = coreElement.phetioFeatured;

assert that phetioFeatured is the same as the core (once both have overrides applied)

That is already set on the last line of the snippet above.

Add logic to studio to make sure that metadata updated in one is updated in the other.

As long as designers know that doesn't auto update, that seems OK, especially given our direction in https://github.com/phetsims/phet-io/issues/1873. Therefore I think this issue can be closed.

[phet-dev]

Reopening because there is a TODO marked for this issue.

[zepumph]

We forgot about #161 (comment), which we ran into over in https://github.com/phetsims/studio/issues/303. Reopening.

[zepumph]

Alright. This is now back in the API files. Yay!

[samreid]

Reopening based on https://github.com/phetsims/studio/issues/303

[zepumph]

From conversations yesterday, we are going to proceed with these commits. They are built into the RC of GO, so we will know if there is any trouble as we go into testing. Closing