Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support uninstrumented PhetioDynamicElementContainers #184

Open
zepumph opened this issue Jun 5, 2020 · 6 comments
Open

Support uninstrumented PhetioDynamicElementContainers #184

zepumph opened this issue Jun 5, 2020 · 6 comments

Comments

@zepumph
Copy link
Member

zepumph commented Jun 5, 2020

Over in https://github.com/phetsims/phet-io/issues/1679, we added code that will opt out of setting phet-io metadata, like phetioType when a PhetioObject isn't instrumented. This broke our implementations of PhetioDynamicElementContainer because they rely on passing phetioType.parameterTypes[0] to createDynamicElement.

I think this is the right direction, despite an error we had in joist demo with an uninstrumented KeyboardHelpButton:

joist : fuzz : unbuilt
https://bayes.colorado.edu/continuous-testing/ct-snapshots/1591393564307/joist/joist_en.html?continuousTest=%7B%22test%22%3A%5B%22joist%22%2C%22fuzz%22%2C%22unbuilt%22%5D%2C%22snapshotName%22%3A%22snapshot-1591393564307%22%2C%22timestamp%22%3A1591396826208%7D&brand=phet&ea&fuzz&memoryLimit=1000
Query: brand=phet&ea&fuzz&memoryLimit=1000
Uncaught TypeError: Cannot read property '0' of undefined
TypeError: Cannot read property '0' of undefined
at PhetioCapsule.create (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1591393564307/tandem/js/PhetioCapsule.js:99:37)
at PhetioCapsule.getElement (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1591393564307/tandem/js/PhetioCapsule.js:67:12)
at KeyboardHelpButton.options.listener (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1591393564307/joist/js/KeyboardHelpButton.js:67:58)
at TinyEmitter.emit (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1591393564307/axon/js/TinyEmitter.js:69:53)
at https://bayes.colorado.edu/continuous-testing/ct-snapshots/1591393564307/axon/js/Emitter.js:33:29
at Emitter.execute (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1591393564307/axon/js/Action.js:225:18)
at Emitter.emit (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1591393564307/axon/js/Emitter.js:58:19)
at PushButtonModel.fire (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1591393564307/sun/js/buttons/PushButtonModel.js:158:23)
at downPropertyObserver (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1591393564307/sun/js/buttons/PushButtonModel.js:90:14)
at TinyEmitter.emit (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1591393564307/axon/js/TinyEmitter.js:69:53)
id: Bayes Chrome
Snapshot from 6/5/2020, 1:46:04 PM

We should work on making sure that these types can be flexible if a tandem is not immediately passed in to them (in phet brand at least).

For right now, I just instrumented the KeyboardHelpButton in the joist demo, this way we don't have this issue.

I think long term we may want to be more flexible, and not access (or be graceful about accessing) phetio properties. This will promote a better library-feel and cause less phet-io "gotchas".

I will add specific assertions right now, and link them to this issue.

@zepumph zepumph self-assigned this Jun 5, 2020
zepumph added a commit that referenced this issue Jun 5, 2020
zepumph added a commit to phetsims/joist that referenced this issue Jun 5, 2020
@zepumph
Copy link
Member Author

zepumph commented Jun 5, 2020

Tagging @samreid and @chrisklus to see what we think the usage of PhetioDynamicElementContainers should be. Are we "allowed" to have uninstrumented ones lying around the project? My instinct is yes, we should support that. I feel this even more strongly due to the assertion above. Why does that instance of KeyboardHelpButton fail in phet brand just because of a phet-io implementation detail.

This may be as easy as making the phetioType an optional arg to createDynamicElement

@samreid
Copy link
Member

samreid commented Jun 8, 2020

Yes, it seems we should allow PhetioGroups to be uninstrumented, we may find reasons to Tandem.OPT_OUT of some common code ones.

@samreid
Copy link
Member

samreid commented Aug 24, 2020

@zepumph can you please take next steps for this issue? Or assign me back if I should.

@samreid samreid removed their assignment Aug 24, 2020
@zepumph
Copy link
Member Author

zepumph commented Aug 27, 2020

It will be a very long time before I get to this.

@samreid
Copy link
Member

samreid commented Feb 3, 2021

I applied a workaround (aka "hack") for this in phetsims/energy-skate-park@6088ad3, where I mocked up the PhetioGroup interface to "uninstrument" it.

@brent-phet
Copy link

Per 12/7/23 PhET-iO Meeting, this is a minor bug and can be addressed if it arises in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants