Closed zepumph closed 1 year ago
I think a workable long-term solution would be to:
PhetioObject.tandem
from Tandem
to Tandem | undefined
(in options and attribute)tandem: options.tandem?.createTandem( 'dragListener' )
Here is a very broken attempt patch. It seems like this would be a lot of work and we should discuss.
I would also recommend that larger issues like this be integrated into our monthly sprint planning. If we decide it is worthwhile and important to the project, it should become a subteam priority rather than something you and/or I look into when we have time.
Tagging for https://github.com/phetsims/phet-io/issues/1914 and self-unassigning.
I actually think that we should close this issue. @samreid we will still want Tandem.OPTIONAL in typescript code so that we can support PhET brand that uses common code, as well as phet-io ports where we want the sim runnable mid-way through the instrumentation process.
Do you want to do more work here?
I feel the above proposal like using tandem?: Tandem
instead of tandem: Tandem
+ Tandem.OPTIONAL
would work well for the optional parts (using optional chaining for usage sites). But I don't see how it could support Tandem.REQUIRED
in a way that supports PhET-iO and non-PhET-iO clients. i.e. we only want to require a tandem if the sim is enabled for PhET-iO. The most refined solution would be if there were different modalities for type checking, like tandem: BRAND_PHET_IO ? Tandem : Tandem | undefined
. But as far as I know there is no way to put conditional types or different modes into TypeScript, and adding that ourselves in another layer sounds horrible. @zepumph any other remarks? Still want to close? Feel free to do so at your discretion.
I'm understanding more now. I think it is worth some investigation about just getting rid of Tandem.OPTIONAL. I tried in a patch like this, and ran into a couple problems:
options.tandem.createTandem()
to subcomponents. I am not really sure how much more effort to put into this. I kinda thought it would be simpler. I am not too interested in options.tandem && options.tandem.createTandem( 'subcomponent' )
in common code. May be we should close it. Please though look at my commit to VarianceNumberProperty, that is nice cleanup, no?
Yes, that is a good change to VarianceNumberProperty. I agree options.tandem && options.tandem.createTandem( 'subcomponent' )
is not good. I feel options.tandem?.createTandem( 'subcomponent' )
is slightly better.
@samreid and I discussed this. We noted that we will not be able to get Tandem.REQUIRED
to go away. This is to support PhET brand code that we don't want to require tandem options for to pass the type checker. We also never want to get rid of Tandem.OPT_OUT
. For Tandem.OPTIONAL, there are 100 usages, and pretty much every one could just be removed if PhetioObject could handle it gracefully.
Though this isn't a high priority or high value item, it does seem obvious to let TypeScript handle this for you in client code.
We can proceed like this:
Subject: [PATCH] move all allowed Tandem characters into the base term character class, https://github.com/phetsims/tandem/issues/270
---
Index: js/createObservableArray.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/createObservableArray.ts b/js/createObservableArray.ts
--- a/js/createObservableArray.ts (revision 83bc8abc096e82c4ff52d7d7343e0b036bd34839)
+++ b/js/createObservableArray.ts (date 1687805583971)
@@ -16,7 +16,6 @@
import merge from '../../phet-core/js/merge.js';
import optionize, { combineOptions } from '../../phet-core/js/optionize.js';
import PhetioObject, { PhetioObjectOptions } from '../../tandem/js/PhetioObject.js';
-import Tandem from '../../tandem/js/Tandem.js';
import ArrayIO from '../../tandem/js/types/ArrayIO.js';
import IOType from '../../tandem/js/types/IOType.js';
import axon from './axon.js';
@@ -95,7 +94,6 @@
length: 0,
elements: [],
- tandem: Tandem.OPTIONAL,
elementAddedEmitterOptions: {},
elementRemovedEmitterOptions: {},
lengthPropertyOptions: {}
@@ -118,7 +116,7 @@
// notifies when an element has been added
const elementAddedEmitter = new Emitter<[ T ]>( combineOptions<EmitterOptions>( {
- tandem: options.tandem.createTandem( 'elementAddedEmitter' ),
+ tandem: options.tandem?.createTandem( 'elementAddedEmitter' ),
parameters: [ emitterParameterOptions ],
phetioReadOnly: true,
hasListenerOrderDependencies: options.hasListenerOrderDependencies
@@ -126,7 +124,7 @@
// notifies when an element has been removed
const elementRemovedEmitter = new Emitter<[ T ]>( combineOptions<EmitterOptions>( {
- tandem: options.tandem.createTandem( 'elementRemovedEmitter' ),
+ tandem: options.tandem?.createTandem( 'elementRemovedEmitter' ),
parameters: [ emitterParameterOptions ],
phetioReadOnly: true,
hasListenerOrderDependencies: options.hasListenerOrderDependencies
@@ -135,7 +133,7 @@
// observe this, but don't set it. Updated when Array modifiers are called (except array.length=...)
const lengthProperty = new NumberProperty( 0, combineOptions<NumberPropertyOptions>( {
numberType: 'Integer',
- tandem: options.tandem.createTandem( 'lengthProperty' ),
+ tandem: options.tandem?.createTandem( 'lengthProperty' ),
phetioReadOnly: true
}, options.lengthPropertyOptions ) );
@@ -264,7 +262,7 @@
/******************************************
* PhET-iO support
*******************************************/
- if ( options.tandem.supplied ) {
+ if ( options.tandem?.supplied ) {
assert && assert( options.phetioType );
observableArray.phetioElementType = options.phetioType.parameterTypes![ 0 ];
``
There are quite a few spots where it is ending up being easier to keep Tandem.OPTIONAL around. For example Carousel, it is just easier to have all variables be of type Tandem
. Something else I was finding though is that the vast majority of usages were actually wanting to use Tandem.OPT_OUT. Now there are only 68 usages left. I would like to at least go through to find more cases where they should be OPT_OUT.
Ok. There are now 26 usages of Tandem.OPTIONAL. That is nice! I deprecated Tandem.OPTIONAL and I'm feeling really good about proceeding without Tandem.OPTIONAL in new code. @samreid can you please review and let me know if you like this path?
I skimmed the commits, reviewed the changed assertion, and reviewed remaining occurrences of Tandem.OPTIONAL. I feel everything is is good shape, thanks! OK to close if you are.
Excellent. I did one more in Input, putting to the test if it is ever worth factoring out Tandem.OPTIONAL for 24 usages of options.tandem?
. Feel free to revert. Closing
I wonder if this is causing a CT error for the LightSpectrumDialog's close button in studio.
Looks like it was actually from https://github.com/phetsims/phet-io/issues/1810. Closing
We will always want Tandem.OPTIONAL for common code instrumentation grace. I'm removing the deprecated markings, as I ran into this in https://github.com/phetsims/scenery-phet/issues/540.
From https://github.com/phetsims/studio/issues/286#issuecomment-1378580398, @samreid and I are likely in agreement about how Tandem.OPTIONAL has run its course. We likely can remove it from everywhere. It was incredibly useful in javascript.
The main concern is where we don't want to break code. Currently, there is always a tandem instance default to call
createTandem
on, but if we remove the defaults, you would have to provide all tandems until you could run the sim. Likely this is a deal breaker unless we can think of a new pattern for this.@samreid can you speak to some ideas about this?