search

phetsims / rosetta

PhET's Simulation Translation Utility
MIT License
3 stars 1 forks source link

Support tsx/typescript/`sage run` with `nodemon` entrypoint #442

Closed zepumph closed 1 week ago

zepumph commented 2 weeks ago

From https://github.com/phetsims/chipper/issues/1525, @samreid and I noticed that perennial code can be imported from app.js via nodemon instead of node. sage run doesn't support this right now, and so we would need to figure this out before we can go all in on typescript in perennial/js/common. We pinged @jbphet for consult, and are creating this issue as to not block 1525 more generally.

zepumph commented 2 weeks ago

Ok. From brief conversation with @jbphet on slack we went ahead and converted this usage from nodemon to node (via sage run).

Totally happy to revert, but I just wanted to formalize my idea. If we are good to proceed with this, likely deleting the "dev-node" script and just using start-node will be best.

@jbphet, let us know what you think, and if you want to meet to chat about it.

jbphet commented 2 weeks ago

@zepumph - When you asked me about this over Slack I was getting ready for another meeting and didn't have time to give it much thought. Honestly, I'd forgotten what nodemon was all about. I just refreshed my memory (thanks Google), and it was being used here because it enables hot reloads of code changes when debugging Rosetta. I believe that this capability could still be quite useful when working on improvements to Rosetta, since it does take a fairly long time to rebuild and start up.

That being said, I certainly don't want this to block progress on https://github.com/phetsims/chipper/issues/1525. In the opening comment for this issue you said, "sage run doesn't support this right now". Might it be possible to support it in the future? If so, I'd propose that we log some sort of error message for now if this is attempted, and schedule the work to add support for this sometime in the next few months. It's not urgent at the moment, since there aren't any currently scheduled improvements to Rosetta on the docket, but I'd also like to have this capability if and when some changes are scheduled.

zepumph commented 2 weeks ago

Sounds good and reasonable. I'll look into it.

zepumph commented 2 weeks ago

Perhaps we can use nodemon as the runnable still

https://stackoverflow.com/questions/37979489/how-to-watch-and-reload-ts-node-when-typescript-files-change

nodemon --watch "src/**" --ext "ts,json" --ignore "src/**/*.spec.ts" --exec "ts-node src/index.ts"

zepumph commented 1 week ago

Alrighty. I believe this will work great. Thanks for pushing back on this. It was a very easy fix. Please spot check and let me know if you have any thoughts. Feel free to close.

jbphet commented 1 week ago

I just testing running the script on my local machine using npm run dev-node, and Rosetta started up fine, and I could see from the output on the terminal that it was indeed using nodemon. I tried making some simple changes to see if they would show up in the UI, but it didn't work, but I don't know if it was working prior to this change since I wasn't using it (I think Liam was). I suspect it wasn't fully operational because the script doesn't seem to rebuild the React code.

I'm going to go ahead and close this and will take the time to make the dynamic reload work correctly the next time we schedule significant work to be done on Rosetta.

Thanks @zepumph for getting this running!