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

Remove Tandem.GroupTandem and previous state pattern #477

Closed
samreid opened this issue Jan 20, 2021 · 2 comments
Closed

Remove Tandem.GroupTandem and previous state pattern #477

samreid opened this issue Jan 20, 2021 · 2 comments
Assignees

Comments

@samreid
Copy link
Member

samreid commented Jan 20, 2021

From phetsims/tandem#87, we need to eliminate usages of createGroupTandem. Things should be converted to use PhetioGroup if necessary. I'll follow the instrumentation guide:

Here is an ordered list of how to approach instrumenting an element that is dynamically created:

  1. Does it even need instrumentation? Often instances don't need to be instrumented, or can perhaps be instrumented as a component of their parent (instead of being instrumented themselves)
  2. Can it be created eagerly? Allocating dynamic elements on startup simplifies the instrumentation process, especially
    when supporting PhET-iO state and API validation.
  3. Instrument the object dynamically, using PhetioGroup or PhetioCapsule. Use PhetioCapsule for single dynamic
    instances. Use PhetioGroup for multiple instances of the same type. If you have a use case that is not addressed by one or both of those, then please consult with the PhET-iO subteam, and potentially create a new IO Type suitable for your simulation.
@samreid
Copy link
Member Author

samreid commented Jan 20, 2021

I reached out to @jessegreenberg on slack to ask if there is ongoing development in this repo that requires coordination.

@samreid
Copy link
Member Author

samreid commented Jan 20, 2021

Fixed in the commits. Once I saw an error like "description should be different if something has changed"? in the state wrapper but I couldn't replicate it.

This sim is listed in the "Upgrade to Studio" goals for instrumentation, I think that will be an appropriate time for improvements, if necessary. But this seems good enough to close. @zepumph reopen if you recommend a review.

@samreid samreid closed this as completed Jan 20, 2021
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

1 participant