What to do when you addLinkedElement and then instrument?

[zepumph]

@samreid were over in https://github.com/phetsims/scenery/issues/1046 and thought it was pretty buggy to have the opportunity to add a linked element before instrumenting for PhET-iO, given what addLinkedElement() currently does (null out this.linkedElements).

There is an assertion to make sure that this doesn't happen:

1. myObject.addLinkedElement();
2. myObject.initializePhetioObject();
3. myObject.addLinkedElement(); // error

But not when you just do 1 and 2. I think we can add an assertion to guard this earlier.

[zepumph]

@samreid what do you think about the above? I added a test to demonstrate the bugginess. My understanding is that no common code that is adding linked elements in master right now would do so out of order, but please check in on CT when you review to double check.

[samreid]

The test and implementation look good. My only recommendation is to eliminate:

// This is to help guard against setting addLinkedElement before initializingPhetioObject.
const LINKED_ELEMENT_PLACEHOLDER = null;

and just use null directly, like so:

```diff Index: js/PhetioObject.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- js/PhetioObject.js (revision a9b45522de229bc2f6b6fbc81fd55c6220993897) +++ js/PhetioObject.js (date 1602519415571) @@ -42,9 +42,6 @@ // Factor out to reduce memory footprint, see https://github.com/phetsims/tandem/issues/71 const EMPTY_OBJECT = {}; -// This is to help guard against setting addLinkedElement before initializingPhetioObject. -const LINKED_ELEMENT_PLACEHOLDER = null; - const DEFAULTS = { // Subtypes can use `Tandem.tandemRequired` to require a named tandem passed in @@ -278,7 +275,7 @@ assert && validate( config.phetioEventMetadata, OBJECT_VALIDATOR ); assert && validate( config.phetioDynamicElement, BOOLEAN_VALIDATOR ); - assert && assert( this.linkedElements !== LINKED_ELEMENT_PLACEHOLDER, 'this means addLinkedElement was called before instrumentation of this PhetioObject' ); + assert && assert( this.linkedElements !== null, 'this means addLinkedElement was called before instrumentation of this PhetioObject' ); // This block is associated with validating the baseline api and filling in metadata specified in the elements // overrides API file. Even when validation is not enabled, overrides should still be applied. @@ -510,9 +507,9 @@ addLinkedElement: function( element, options ) { if ( !this.isPhetioInstrumented() ) { - // set this to null so that you can't addLinkedElement on an uninitialized PhetioObject and then instrumented + // set this to null so that you can't addLinkedElement on an uninitialized PhetioObject and then instrument // it afterwards. - this.linkedElements = LINKED_ELEMENT_PLACEHOLDER; + this.linkedElements = null; return; } @@ -534,7 +531,7 @@ * @public */ removeLinkedElements: function( potentiallyLinkedElement ) { - if ( this.isPhetioInstrumented() && this.linkedElements !== LINKED_ELEMENT_PLACEHOLDER ) { + if ( this.isPhetioInstrumented() && this.linkedElements ) { assert && assert( potentiallyLinkedElement instanceof PhetioObject ); assert && assert( potentiallyLinkedElement.isPhetioInstrumented() ); ```

If you do decide to keep a named constant for this, a better name would be nice. Perhaps LINKED_ELEMENTS_DISABLED or something?

but please check in on CT when you review to double check.

I didn't see any CT errors that I could attribute to this change.

[zepumph]

Sounds good to me. Thanks!