Update example-sim to current standards

phetsims / example-sim

Example demonstrating the structure of a PhET interactive simulation.

http://scenerystack.org/

GNU General Public License v3.0

14 stars 27 forks source link

Update example-sim to current standards #19

Closed fgamador closed 1 month ago

fgamador commented 3 months ago

As an exercise to help me learn SceneryStack, I updated example-sim to match what seem to be the current standards. In the hopes that this will help future new devs, here are my changes. Please adjust as needed to fix things I got wrong. Here's the summary:

Converted to TypeScript.
Switched from using merge to optionize. (Not sure I handled ExampleSimConstants.SCREEN_OPTIONS correctly.)
Added and used ExampleSimColors file.
Added/updated other files to match simula-rasa. (I generated a new, empty example-sim using create-sim and diffed against it.)
Fixed lint errors, including removing javadoc-style @param and @return keywords. Not sure what should replace these.

Note that while I tried to match existing formatting, I don't have a WebStorm license, so I didn't run an auto-formatter. Is there a way to do that without buying WebStorm?

pixelzoom commented 3 months ago

Adding this to the Developer Priorities board, to be discussed at an upcoming PhET developer meeting.

Thanks for the work here @fgamador, this looks very nice.

Before accepting this pull request, we'll need to revisit whether PhET wants example-sim to be in TypeScript. The current answer was "no". Back when I doing a lot of proactive conversion from JavaScript to TypeScript (2022?), PhET decided not to convert to example-sim to TypeScrpt because:

(1) example-sim is used for programming tasks during PhET's hiring process, where JavaScript experience is a prerequisite, but TypeScript is not. Requiring TypeScript experience would significantly reduce the number of qualified candidates.

(2) For the open source community, TypeScript may be an additional barrier to getting involved with PhET code. The PhET tool chain certainly requires TypeScript, but it's still possible to write a sim entirely in JavaScript.

I think these points are both worth revisiting.

fgamador commented 3 months ago

Okay, makes sense. I was wondering whether there was a reason example-sim is still JavaScript, but then I saw TypeScript requirements elsewhere. Whether or not you accept this, I found the conversion a valuable exercise, especially since I haven't used TypeScript before, and my JavaScript is pretty rusty.

marlitas commented 3 months ago

Hi @fgamador, we decided that we are ready to convert example-sim to typescript and are very grateful for your work here! I will be reviewing the code in the next few days and will send any questions or revisions your way by early next week so we can merge your pull request in.

fgamador commented 3 months ago

Okay, sound good. Glad to be able to help.

fgamador commented 3 months ago

Thank you, @marlitas, for your thorough review. I have one other item I've been wondering about, which is the jsdoc annotations. I removed (most of?) them because when I ran lint from grunt, it complained about them. But now I'm unsure that that was the right thing to do. They are partly redundant to declarations in TypeScript, such as @public and parameter types, but maybe not entirely. Do you have a standard on how TypeScript should be commented?

marlitas commented 3 months ago

I removed (most of?) them because when I ran lint from grunt, it complained about them. But now I'm unsure that that was the right thing to do.

@fgamador, yes that is correct! We do not document the redundant typescript information in jsdoc, since it can quickly become a nuisance to maintain and update two spots at once. Generally the addition information jsdoc will provide is specific to the implementation or context of a function, parameter, class field, etc.

Overall your jsdoc looked as I would expect and no needed changes were apparent there.

fgamador commented 2 months ago

@marlitas, any thoughts on my other questions?

marlitas commented 2 months ago

@fgamador so sorry! I didn't even notice those. I'll take a look later today and respond. thanks for the ping!

marlitas commented 2 months ago

@fgamador I believe I responded to all of your questions, but please let me know if I missed any!

fgamador commented 2 months ago

Okay, I think I've addressed all your comments, @marlitas. Sorry for the slow response; I've been out of town. And I'm heading out again, so it'll be another week before I can respond again.

marlitas commented 2 months ago

@fgamador no worries. It will take a moment for me to look through it and review. Thank you so much!

marlitas commented 1 month ago

@fgamador we were successfully able to merge everything in and it's all working and looking as expected. Thanks again for all your work here!