-
Notifications
You must be signed in to change notification settings - Fork 5
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 types to use PhetioObject #46
Comments
Beer's Law Lab is now using PhetioObject, see phetsims/beers-law-lab#219 |
I've done several, but there are still 30 or so more cases to convert. Tagging @ariel-phet and @kathy-phet so they are aware I'll be working on this. |
…ctDisposed to PhetioObject.isDisposed, see phetsims/tandem#46
…ctDisposed to PhetioObject.isDisposed, see phetsims/tandem#46
…ctDisposed to PhetioObject.isDisposed, see phetsims/tandem#46
…ctDisposed to PhetioObject.isDisposed, see #46
…ctDisposed to PhetioObject.isDisposed, see phetsims/tandem#46
Presumably yes. Can test in the Scenery playground: function SubNode() { scenery.Node.call( this ); }
phetCore.inherit( scenery.Node, SubNode, {
dispose() { console.log( 'oops no super call' ) }
} );
var s = new SubNode();
s.dispose(); // no error however if it's extremely rare to run into this for all devs, then I'm fine dropping it. Presumably the code could just be moved over to PhetioObject? (I'll test that and see if I run into assertions anywhere). |
The other commits (tandem/axon/scenery) look good. |
Adding the assertion found that ESPB/ESP Track.js inherits PhetioObject but doesn't do the super dispose. |
That sounds like a good catch, do you want to commit the change to PhetioObject? |
@samreid will commit the fix, then remove the "blocks" label. Wait on GQ and AM RCs until this is done. |
Proposed fixes above and correctly flag problems in Energy Skate Park sims. Flagging @jessegreenberg so he is aware. Reassigned to @jonathanolson to review and close if all is well. |
Increasing priority, since this is blocking Graphing Quadratics 1.0 RC. Please notify me when this is closed. |
I also mentioned to @pixelzoom that he can review this if he gets to it first. |
I'd prefer not to spend time familiarizing myself with this issue. But if @jonathanolson doesn't get to it by Monday morning, I'll dive in. |
Looks good to me. |
I took a look too, had some questions on Slack, was convinced that this has low risk to cause problems. |
…ctDisposed to PhetioObject.isDisposed, see phetsims/tandem#46
See https://github.com/phetsims/phet-io/issues/1227
The text was updated successfully, but these errors were encountered: