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

Enable TypeScript in NodeJS code #1437

Closed
9 of 25 tasks
samreid opened this issue Apr 12, 2024 · 20 comments
Closed
9 of 25 tasks

Enable TypeScript in NodeJS code #1437

samreid opened this issue Apr 12, 2024 · 20 comments

Comments

@samreid
Copy link
Member

samreid commented Apr 12, 2024

I swamped #1272 with commits, so let's continue over here. In summary, we would like to enable TypeScript usage in perennial & chipper. Recent dev meeting notes are in: https://docs.google.com/document/d/11Gt3Ulalj0fCD2fFeCjPT5ni_9mM2WjkGc4ysisQmo8/edit

A summary is in #1272 (comment)

  • @zepumph is doing regular work in perennial and chipper and feels like it would be a significant productivity improvement to have the code in TS.
  • @samreid feels that he has taken it about as far as he can on his own and would like help from either @jonathanolson, @pixelzoom, or @zepumph.

The general conclusion is that we should definitely go for an incremental process, and @samreid will review everyone's input in the developer notes doc and we will subsequently discuss the incremental plan, and @marlitas will get this item on the planning list for the next iteration. (UPDATE: or the current iteration)

From #1272 (comment)

Also, the revelation that we can use import statements anywhere in the chain and everything will still work because the transpiler outputs require statements that grunt can consume.

Current checklist from #1272 (comment)

Next steps:

  • Apply the patch above
  • Address some of the TODOs, such as standardizing the status cache keys
  • Notify the dev team that changes are incoming
  • Commit and push to main
  • Test on Windows
  • Notify team members that the transpiler is now outputting a commonjs/requirejs format for some repos. Notify that cache uses different keys and will be invalid. Recommend restarting transpiler with --clean. Notify to be on the lookout for gruntMain issues.
  • Point Gruntfile entry points at gruntMain
  • test a chains build/deploy process
  • test on Windows again
  • adjust tsconfigs to optimize for usage in node
  • port 1-2 grunt files to typescript minimally
  • move gruntMain to perennial-alias
  • move perennial to use perennial/gruntMain instead of js/grunt/Gruntfile.js
  • Support other non chipper gruntfiles (aqua/rosetta/etc)
  • Support gruntfiles that use chipper AND register their own tasks (dot/kite/scenery)
  • port 1-2 non-grunt files minimally, and consider how we should launch it. (ts-node/deno/custom)
  • How do we determine what the list of build-tool dependencies are to transpile? aqua needs perennial, chipper needs perennial-alias?
  • toggle gruntMain to use typescript
  • sourcemaps? how will stack traces look?
  • more complete porting of grunt and non-grunt files in chipper
  • porting some files in perennial and perennial alias
  • what if we want to load Property.ts in chipper? What would that take?
  • For winston, we could use a babel plugin or output to repo/dist
  • Several TODOs in the code (9 on 5/15/24)
  • Can we use tsc for our transpiling yet? Probably not, and likely it shouldn't be part of this build-tool work, but it may be worth investigating. Babel seems good and fast enough for our purposes now.
@samreid
Copy link
Member Author

samreid commented Apr 12, 2024

@zepumph and I reviewed the current status and agreed it is at a good point to put on hold until we have more bandwidth to "flip the switch".

marlitas added a commit to phetsims/mean-share-and-balance that referenced this issue Apr 12, 2024
samreid added a commit that referenced this issue Apr 16, 2024
@zepumph
Copy link
Member

zepumph commented May 6, 2024

We want to prioritize #1356 first.

@zepumph
Copy link
Member

zepumph commented May 15, 2024

@samreid and I met today to discuss this further. We had the following discussions:

  1. ts-node may be worth investigating to drive our node-based typescript, but we were totally thwarted even by a hello world, and see no path forward to getting it to work with grunt.
  2. Transpiling with our custom transpiler into two modes seems good and reasonable as a step forward. We don't need to investigate other strategies to unblock this issue (but could at a different time independently). TSC will most likely never be as fast as the babel script we are using in Transpiler.js.

@zepumph
Copy link
Member

zepumph commented May 15, 2024

What will it take to commit flipping the feature flag in gruntMain?

  • confirm that grunt options work
  • confirm that tasks registered by a "sub gruntfile" work (like aqua's quick-server task)
  • cd buoyancy; grunt/ gave this problem:
 mjkauzmann ~/PHET/git/buoyancy (main)
 $ grunt --lint=false
Running "report-media" task

Running "clean" task

Running "build" task
Fatal error: Perennial task failed:
Error: ENOENT: no such file or directory, open 'C:\Users\mjkauzmann\PHET\git\chipper\dist\commonjs\perennial-alias\data\active-repos'
    at Object.openSync (node:fs:601:3)
    at Object.readFileSync (node:fs:469:35)
    at Object.<anonymous> (C:\Users\mjkauzmann\PHET\git\chipper\dist\commonjs\chipper\js\grunt\webpackBuild.js:29:22)
    at Module._compile (node:internal/modules/cjs/loader:1254:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1308:10)
    at Module.load (node:internal/modules/cjs/loader:1117:32)
    at Module._load (node:internal/modules/cjs/loader:958:12)
    at Module.require (node:internal/modules/cjs/loader:1141:19)
    at require (node:internal/modules/cjs/helpers:110:18)
    at Object.<anonymous> (C:\Users\mjkauzmann\PHET\git\chipper\dist\commonjs\chipper\js\grunt\buildStandalone.js:26:20)
Full Error details:
Error: ENOENT: no such file or directory, open 'C:\Users\mjkauzmann\PHET\git\chipper\dist\commonjs\perennial-alias\data\active-repos'

@samreid
Copy link
Member Author

samreid commented May 15, 2024

It seems like data/active-repos is accessed through a relative path. Two ideas:

  • Should we copy perennial/data files to chipper/dist?
  • Should we change the code so it can find active-repos and other data files no matter where things are run from?

UPDATE: There are 12 occurrences of __dirname in chipper. Perhaps convert those to a path relative to the runtime directory?

zepumph added a commit that referenced this issue May 15, 2024
Signed-off-by: Michael Kauzmann <[email protected]>
zepumph added a commit that referenced this issue May 15, 2024
Signed-off-by: Michael Kauzmann <[email protected]>
@samreid
Copy link
Member Author

samreid commented May 15, 2024

I added some commits in a branch. After the change above, grunt lint succeeds in buoyancy.

@samreid samreid removed their assignment May 17, 2024
@zepumph zepumph removed their assignment Jun 5, 2024
@samreid
Copy link
Member Author

samreid commented Jul 25, 2024

Node.js is getting native TypeScript support as part of https://github.com/nodejs/node/pull/53725. We should put efforts on hold until that is more stable and we can evaluate it.

UPDATE: The node documentation also refers to https://tsx.is/ for more complete but heavyweight support.

@samreid samreid self-assigned this Jul 29, 2024
@samreid
Copy link
Member Author

samreid commented Sep 7, 2024

I tested on Windows and confirmed that grunt --brands=phet-io is working well with the transpiled chipper grunt code.

samreid added a commit that referenced this issue Sep 11, 2024
samreid added a commit that referenced this issue Sep 11, 2024
samreid added a commit that referenced this issue Sep 11, 2024
samreid added a commit that referenced this issue Sep 11, 2024
samreid added a commit that referenced this issue Sep 11, 2024
samreid added a commit that referenced this issue Sep 11, 2024
@samreid
Copy link
Member Author

samreid commented Sep 12, 2024

#1459 is now the most promising way forward. Let's put this issue on hold until that issue is more fully explored. Note that we will likely want to revert some changes here, namely in the transpiler which is overcomplicated, once we are more confident in #1459

@samreid
Copy link
Member Author

samreid commented Sep 13, 2024

After consultation with @zepumph, I feel the best way forward is the proposal in #1459. I have reverted the various aspects for dual-mode transpiling for this issue, which were clumsy, slow, and overcomplicated. I notified slack dev-public to pull and restart transpiler watch processes. There are no more TODOs pointing to this issue. Let's close this issue and continue in #1459.

@samreid samreid closed this as completed Sep 13, 2024
@samreid
Copy link
Member Author

samreid commented Sep 13, 2024

Well, let's keep this open (and don't delete the branch yet) since the branch has a lot of commits converting chipper/js/grunt/ files to typescript.

@samreid samreid reopened this Sep 13, 2024
samreid added a commit that referenced this issue Sep 13, 2024
@samreid
Copy link
Member Author

samreid commented Sep 16, 2024

We recovered the commits and brought them into #1459. Branch deleted. Closing.

@phet-dev
Copy link
Contributor

phet-dev commented Nov 5, 2024

Reopening because there is a TODO marked for this issue.

@samreid
Copy link
Member Author

samreid commented Nov 6, 2024

Fixed and will be merged to main with SWC, closing.

@samreid samreid closed this as completed Nov 6, 2024
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

4 participants