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

Convert PhetioDynamicElementContainer and subclasses to TypeScript #254

Open
samreid opened this issue Feb 11, 2022 · 5 comments
Open

Convert PhetioDynamicElementContainer and subclasses to TypeScript #254

samreid opened this issue Feb 11, 2022 · 5 comments

Comments

@samreid
Copy link
Member

samreid commented Feb 11, 2022

For phetsims/center-and-variability#26, we would like PhetioGroup in particular to be in TypeScript. I'll take a look. Also noting that converting common code to TypeScript is one of my 2022 Q1 goals.

@samreid samreid self-assigned this Feb 11, 2022
@samreid
Copy link
Member Author

samreid commented Feb 11, 2022

This also benefited from converting Emitter and Action to TS. I followed precedent in TinyEmitter, and that worked OK. There are several TODOs, any and ts-ignore in the PhetioDynamicElementContainer classes, but it's providing much better type support in simulation usage (as tested in CAS). The main issue that should be worked on next is inference of the creation args--right now it is fully explicit and must be specified manually, like this.cardModelGroup = new PhetioGroup<CardModel, CASObject>. I'll commit what I have since type checking is passing and aqua seems reasonable.

@samreid
Copy link
Member Author

samreid commented Feb 11, 2022

@jonathanolson and I did a mini-review and we think the scenery changes are good. We also think the array pattern from TinyEmitter will work for the PhetioGroup and related classes.

samreid added a commit to phetsims/center-and-variability that referenced this issue Feb 11, 2022
samreid added a commit to phetsims/center-and-variability that referenced this issue Feb 11, 2022
samreid added a commit to phetsims/circuit-construction-kit-common that referenced this issue Feb 11, 2022
samreid added a commit to phetsims/circuit-construction-kit-common that referenced this issue Feb 11, 2022
samreid added a commit to phetsims/axon that referenced this issue Feb 11, 2022
samreid added a commit to phetsims/greenhouse-effect that referenced this issue Feb 11, 2022
samreid added a commit to phetsims/greenhouse-effect that referenced this issue Feb 11, 2022
@zepumph
Copy link
Member

zepumph commented Apr 18, 2022

I have some of these next steps in my working copy. Commits coming soon.

zepumph added a commit to phetsims/circuit-construction-kit-common that referenced this issue Apr 18, 2022
@zepumph
Copy link
Member

zepumph commented Apr 18, 2022

The majority of this work is as follows:

  • Update PhetioDynamicElementContainer to recent standards (remove @public/private/returns jsdoc, optionize options pattern, etc)
  • Support this.archetype sometimes being null. The involved creating a getter that asserts that it is null. Many usages of defaultArguments needed to change to have a closure to ensure things existed by then. I'd call that a fair few bug fixes!
  • I typed out PhetioObjectMetadata which was a bit of a mess, but will help us greatly when we move into core PhET-iO modules to convert them to TypeScript.
  • phetioEventMetdata has a more specific option type.

@samreid, Can you take a look and see if you see more to do in PhetioDynamicElementContainers and PhetioObject?

zepumph added a commit to phetsims/energy-forms-and-changes that referenced this issue Apr 18, 2022
zepumph added a commit to phetsims/center-and-variability that referenced this issue Apr 18, 2022
zepumph added a commit to phetsims/charges-and-fields that referenced this issue Apr 18, 2022
samreid added a commit that referenced this issue May 9, 2022
…nal and improve type for PhetioGroupOptions, see #254
@samreid
Copy link
Member Author

samreid commented May 9, 2022

@marlitas and I made improvements in the commit, but ran into one problem with phetioDynamicElementName. @zepumph can you please review the TODO and ts-ignore and recommend how to proceed? It is about needing multiple optionize calls since one optionize uses the results of other optionize. (Or maybe we can find a way to do it all in the one optionize).

@samreid samreid removed their assignment Aug 13, 2022
@zepumph zepumph removed their assignment Mar 3, 2023
@zepumph zepumph self-assigned this Jun 23, 2023
@zepumph zepumph removed their assignment Jul 6, 2023
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

2 participants