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

PHET_IO in molecule-polarity-config.js? #43

Closed
pixelzoom opened this issue Jul 12, 2017 · 11 comments
Closed

PHET_IO in molecule-polarity-config.js? #43

pixelzoom opened this issue Jul 12, 2017 · 11 comments
Assignees

Comments

@pixelzoom
Copy link
Contributor

pixelzoom commented Jul 12, 2017

As part of code review #30, @zepumph removed PHET_IO from molecule-polarity-config.js in 6e226e1. But I see that @samreid added it in f1205f4, and apparently did so for all {{REPO}}-config.js files, https://github.com/phetsims/phet-io/issues/453. And PHET_IO is still in {{REPO}}-config.js for other sims that are not instrumented (e.g. function-builder-config.js).

I'm not clear on why it was added by @samreid. @zepumph please investigate whether it needs to be restored.

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Jul 12, 2017

Btw... If it's no longer needed here, then that's a more general issue -- it should probably be removed from all {{REPO}}-config.js files where it's not needed.

@samreid
Copy link
Member

samreid commented Jul 12, 2017

I tried removing PHET_IO from the molarity config.js file and after doing so was unable to launch phet-io-wrappers/instance-proxies/instance-proxies.html?sim=molarity&ea

because this GET fails:

require-2.1.11.js:1895 GET http://localhost/molarity/js/PHET_IO/SimIFrameAPI.js?bust=1499897604691 

I suspect it is also necessary for PhET-iO builds. Does this seem reasonable?

@samreid samreid removed their assignment Jul 12, 2017
@pixelzoom
Copy link
Contributor Author

@samreid asked:

Does this seem reasonable?

I don't know. This sim isn't instrumented. Should we wait to add PhET-iO dependencies as part of the instrumentation process?

@pixelzoom
Copy link
Contributor Author

... and what was the motivation for adding PhET-iO dependencies to config.js for all sims?

@zepumph
Copy link
Member

zepumph commented Jul 12, 2017

I don't know. This sim isn't instrumented. Should we wait to add PhET-iO dependencies as part of the instrumentation process?

That seems reasonable to me. Isn't molarity instrumented though? I would expect it to fail in phet-io mode.

@samreid
Copy link
Member

samreid commented Jul 13, 2017

Should we wait to add PhET-iO dependencies as part of the instrumentation process?

We wanted to make it so that simulations could do PhET-iO record and playback without any instrumentation. However, it did require adding PHET_IO to the config.js files.

@pixelzoom
Copy link
Contributor Author

Labeled for developer meeting, so we can get a consensus on when to add PHET_IO to config.js.

@zepumph
Copy link
Member

zepumph commented Jul 13, 2017

We wanted to make it so that simulations could do PhET-iO record and playback without any instrumentation. However, it did require adding PHET_IO to the config.js files.

It seems like there is always a manual step before getting a sim ready for record and playback. This is because not all ui components are instrumented for the data stream, so we have to make sure that everything is being emitted that we want. It seems reasonable that we would just add the PHET_IO dependency during that step, instead of always having it with every sim.

I would even argue that having it on all sims could be confusing, because someone may take that as a sign that the sim is ready for record/playback when it isn't.

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Jul 13, 2017

My vote would be to add ifphetio and PHET_IO to config.js when PhET-iO features are needed. And indicate that in https://github.com/phetsims/phet-io/blob/master/doc/how-to-instrument-a-phet-simulation-for-phet-io.md.

@pixelzoom
Copy link
Contributor Author

7/13/17 dev meeting:
• restore PHET_IO in molecule-polarity-config.js
• for config.js checklist item, indicate that all config.js files will have PHET_IO and ifphetio

@zepumph will handle.

@zepumph
Copy link
Member

zepumph commented Jul 13, 2017

in 4196982 I reverted the dependency update. I also updated the checklist above. Closing.

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

3 participants