search

phetsims / neuron

"Neuron" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
1 stars 3 forks source link

Convert to es6 #146

Closed zepumph closed 3 years ago

zepumph commented 3 years ago

From https://github.com/phetsims/tasks/issues/1051

zepumph commented 3 years ago

When I did the conversion, there were 145 lint errors, most of which was accessing this before super. I'll take a first pass, and then assign over to responsible dev.

zepumph commented 3 years ago

Lots was done in the above commit. The majority of the lint errors fixed above were adding visibility annotations to methods in RecordAndPlaybackModel, as well as making sure that this wasn't used before super is.

There was one workaround I had to put into place because of how NeuronModel called to super after initializing all of its member vars. Since its supertype, RecordAndPlayback, called many overridden methods in its constructor, it wasn't enough to just move the super call to the beginning of NeuronModel's constructor. Instead I have to create an initalize function, and call that at the end.

One of two things should probably happen:

  1. Get rid of the initialize function and make it so NeuronModel can work by just calling super when it should. (obviously preferred).
  2. Decide we need to keep around the initialize function (marked with a TODO pointing to this issue). In this case, assertions should be added to every function to make sure they aren't called before initialization. Also this.initialized should be set to false in the constructor.

There are also some TODOs that mark unused methods. I didn't know if they were purposefully there (like to complete the API for RecordAndPlaybackModel.js), or if they should be removed.

@jbphet, I hope I didn't overstep in the work I did here. This was a bit more challenging than any other sim I had encountered, but I felt like I was in deep enough that it was worth plowing forward. Let me know if I can do anything more here.

jbphet commented 3 years ago

@zepumph - I removed the initialize function in RecordAndPlaybackModel by moving pretty much all of its contents to the constructor. It seems to have been fairly straightforward and worked fine, so I'm a little suspicious that I misunderstood something. Can you take a quick look and make sure that there isn't something that I missed that you had seen that necessitated the initialize thing?

Also, I deleted the TODO markers in RecordAndPlaybackModel and left the methods in place. This is actually a common code class that was ported from the Java version, and while at this point I suspect it will never be used for another sim, I feel like we might as well leave the API complete just in case.

zepumph commented 3 years ago

I was puzzled for a second, but then I saw that you also removed the usage of resetAll() from the end of the constructor of RecordAndPlaybackModel. This called clearHistory, which is overridden by NeuronModel and thus tries to access transientParticlesBackup from RecordAndPlaybackModel's constructor before it has been assigned in NeuronModel.

I feel a bit bad for doing this es6 conversion, I hope it still saved your time as opposed to taking more of it.

I'm ready to close if you are.

jbphet commented 3 years ago

@zepumph - it definitely saved me time, and I appreciate your efforts on it.

I think we're ready to close this.