should we use a tandem "marker entry" in options?

[pixelzoom]

Noted in #259 (comment)

I had some confusion about whether ArrowButton was instrumented, and said:

No tandem appears in the ArrowButton source code, but it is passed through to its supertype (RectangularPushButton), where it appears to be handled properly.

Looking at ResetAllButton (a subtype of RoundPushButton), I see:

tandem: null // Marker entry to indicate that tandem is supported (in the parent)

Should ArrowButton have a similar "marker entry" in its options? Is this a convention that we're going to use?

[pixelzoom]

In #259 (comment), @samreid said:

I have mixed feelings about the marker and would like to discuss it before we decide. I suppose at the moment I'm leaning toward "delete them because they are duplicated information and it would be crazy to have marker entries for everything" though I do see the advantage of being able to easily see if something is instrumented in a parent class. Perhaps there is a better way to proceed.

[pixelzoom]

Labeled for discussion at developer meeting.

[samreid]

@jbphet points out this is just a holdover until tandem is supported broadly.

We would rather not duplicate options, and we should remove the marker entries.

[samreid]

Marked with high-priority to put it on my todo list.

[samreid]

If we decide to proceed with non-null tandems in common code options as discussed in https://github.com/phetsims/phet-io/issues/562 then perhaps it makes sense to declare tandem: Tandem.createOptionalTandem('myClass') at each point in the hierarchy--the subclassiest one will take effect. Should be discussed with @andrewadare before proceeding.

[samreid]

@andrewadare the pattern that we discussed today sounds good, and in a subclass hierarchy, each class will specify tandem: Tandem.createDefaultTandem('myClass'). This is in effect like a "marker entry" but one that will work well with our non-null tandem pattern. I'll unassign but leave open and we can re-evaluate after https://github.com/phetsims/phet-io/issues/562 is complete.

[pixelzoom]

@samreid @andrewadare Is this issue still relevant?

[andrewadare]

Yes, thanks for the reminder. In fact it is more relevant now that phetsims/phet-io#562 is done. I will flag for discussion with other PhET-iO devs.

[andrewadare]

Noting here for future reference: FaucetNode.js in scenery-phet is a nice example to follow. It includes the createDefaultTandem call, good comments, and disposal code.

[samreid]

The pattern in ArrowNode is a good pattern, with the caveat that TArrowNode should be used instead of TNode.

There are 13 tandem: null in sun and 9 in scenery-phet. 43 across all of our repos (including sun and scenery-phet).

To do this right, we should:

• replace tandem: null with tandem: tandem.createDefaultTandem('myType')
• make sure all sites have tandem.addInstance()
• make sure all sites have tandem.removeInstance() implemented in a private disposeMyType function, which is called from the public dispose function.
• add Tandem.validationOptions(options)

Before tackling this, we should:

• augment automated testing so it will test phet-io versions so we can see where tandems are missing, see https://github.com/phetsims/phet-io/issues/158

The latter checkbox needs a separate issue.

[samreid]

On hold until https://github.com/phetsims/phet-io/issues/158 is complete.

[samreid]

The problem is that scenery-phet and other libraries are still using tandem: null, so when subclassing a sun component, the tandem is passed in as null, but sun expects it to be non-null.

[samreid]

After the above commits, aqua requirejs tests are passing again for PhET brand.

[zepumph]

I updated all tandem: nulls in scenery-phet

[pixelzoom]

@samreid @zepumph What is the status of this issue? Is there more work to be done here? Something to review? Ready to close?

[zepumph]

Thanks @pixelzoom for bring it back up.

[zepumph]

I got rid of the last two spots that had "tandem:null" (Property and ObservableArray) in the project. This helped to catch a bug in MAL as well, where tandems weren't being passed into Photon.

Here are the related commits,

tandemOptional in Property.js, factor Optional/RequiredAndNotSupplied into required and supplied options
phetsims/tandem@294a42d

instrument MAL after observableArray update
phetsims/molecules-and-light@89857f6

tandemOptional in Property.js
phetsims/axon@3471b08

tandemOptional in ObservableArray.js
phetsims/axon@2a21804

@samreid would you please look over these commits and make suggestions. I think this is probably ready to close after review.

[zepumph]

@samreid here I think a review just needs to look at the above commits for fowl play. Check to make sure that tandem: null was replaced correctly.

[samreid]

The preceding commits look great, thanks! Closing.