-
Notifications
You must be signed in to change notification settings - Fork 78
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
Lf 4395 setup typescript on backend #3428
base: integration
Are you sure you want to change the base?
Conversation
714f667
to
84f3923
Compare
Wow, thank you so much for putting this together! The PR description was super helpful to understand the changes and the decision making process, amazing work 👏 I tested it out and it all seems to be working properly -- for the Dockerfile, I'm assuming we'll need to update the Node version and the last piece to remove installing nodemon and using it to start the server. I'm thinking we could check out this branch on the beta server and test the changes. Right now we're using the same Node version for all the packages, so I think it might be good to update all of them to 20 which is LTS anyway. We should also update the Readme instructions on the parts it refers to Nodemon. |
@antsgar Thank you for testing this out! I'm a bit confused with the dockerFile and what its meant to do. It's named as prod.DockerFile which suggests its for production, but then its using nodemon which is only meant for development. So if we remove nodemon from the dockerFile, then does it needs to be replaced with |
Hey @navDhammu sorry it has taken me some time to get to this, I am still excited this could be available soon! (also tagged Sayaka and Joyce as reviewers to get their eyes on this big improvement!) The npm settings for I tested and everything worked well on:
I tested and it fails:
I expect to fail
So my other current thoughts are:
|
…ns files, api tests and webapp tests
I've made the changes for updating node version in all packages, as well as replacing nodemon in the prod.Dockerfile. I had to update the pnpm lock file in webapp by running pnpm install because after updating node version in the github actions it was failing. After some more searching around the codebase I found 2 script files Export server failing in docker compose:
Typescript version:
Tsconfig files:I tried using a single tsconfig but the problem is you can't pass the tsconfigs include option in the cli. So if we use the main /tsconfig then tests will get compiled in the build which we dont want, and if we use the other one then only src will get typechecked in precommit. Import syntax and .js vs .ts:--allowImportingTsExtensions only works when used with --noEmit, and we need to use emit in order to compile the /dist bundle for production. |
Sorry if I was unclear on a couple things! Node version I think we want to set the explicit version of node 20 something -- not --lts. I think our last tech lead Ivan stressed the importance of not setting a moving target for core engines to avoid surprises down the road and have a known dependency list. It is handy to not update... but could potentially break things without a traceable action on our side. nodemon
For your questions Export server Ahh true I forget that is there sometimes -- will ask Joyce if there is a best way to update those and make a ticket. Typescript Agreed on long term solution Tsconfig files We can go with two for now -- I think we typically only need to Import .js vs .ts Yah I might be wrong on that and not a big deal really to keep -- but I think babel (or in this case swc) should have an extension/plugin like --rewriteImportExtensions for either using |
@Duncan-Brain I agree on setting the node version to an exact number like 20.18, I realized this afterwards when the CI was failing due to pnpm-lock error which happened because the node version changed. Regarding nodemon:
Tsconfig:
For typescript version:
|
Node version 👍 Nodemon -- right it would need to have the build command first then it would change to something using the Tsconfig - a) no worries about figuring it out now! b) I asked because like the node version it could change without us knowing -- just nice to be in charge of base config and reference where needed maybe -- open to either way though Typescript version - a) I don't know how much work that would be so maybe -- the suggestion to make a prequel PR with just node and typescript might help separate things nice but no obligation just things I am noticing b) pnpm workspaces -- Sounds interesting -- I think we were looking at migrating back to |
Nodemon - Yeah it has to be built first then run through the dist folder, or if its for dev only then can be run directly with swc-node. The reason I suggested workspaces is because its very easy to setup and would fix the typescript version issue for good, among others. The amount of work to make a prequel PR with node and typescript version for every package would be about the same as setting up something like pnpm workspaces. All that's needed is to create a pnpm-workspace.yaml file, and adjust scripts from npm to pnpm, then install typescript in the root and it'll be available for each package in the same version. From my experience npm's workspace feature isn't as good compared to pnpm workspace having tried both - I found pnpm to be better in terms of speed and ease of use. npm also has some other issues like being able to import packages you didn't insall, for example, it's possible in npm to do something like |
I created a proof of concept PR #3523 for switching from npm to pnpm workspaces which solves the above typescript version issue, among other handy features. Would appreciate your thoughts and comments when you have the time. |
This sets up typescript on the backend. New and existing files in
src
andtests
should work now when converted to .ts. Anything in/db
including the migrations still need to use .js but that can be setup with typescript as well if needed. I tested it to make sure its good for development and adjusted the npm scripts for production and integration, but will need some help to ensure nothing is broken in production as I'm not fully aware of how everything is setup with docker, for example there's prod.DockerFile which uses some npm commands that may not work.Overview
In order to get typescript working on the backend, it has to first be transpiled to js so it can be executed by node as node doesn't support running ts files natively. Although that may change in the near future with this new experimental feature in latest release.
Tsc is the official compiler shipped with typescript that does the transpilation from ts to js, and it's what I started working with initially but then switched to using swc after running into problems with jest. Swc is an alternate to tsc that's written in rust so its much faster and provides a better dev experience, there are others like esbuild which is used by vite on the frontend. I chose swc because they provide a package swc/jest that takes care of the problems I was having initially getting typescript and es modules working with our current test setup with jest. A nice side effect of using this package is that tests are running faster now, I'm seeing api tests in github actions completing in about 3 minutes instead of 6.
For development, theres still this issue of having to first compile all source code to .js before running the node server. So the workflow needs to be: make a change -- run the compiler -- execute the compiled server.js file. To make all this more straightforward there's this popular package ts-node that allows executing .ts files directly, so it makes it possible to directly execute
ts-node server.ts
. It does this by compiling typescript to javascript on the fly and passing the resulting javascript code to node. I found a similar well maintained package swc-node that does the same thing but uses the swc compiler, so I ended up going with that.To make things fast during development, this setup doesn't run type-checking on every code change, but instead its done in the pre-commit just like on frontend.
Changes
engines
option in package.json andengine-strict=true
in.npmrc
. So when using an npm command like npm install, it will give an error if using the node version doesn't match this.npm run dev
replacesnpm run nodemon
. Nodemon is not needed as node.js comes with watch mode after node v18/dist
via tsc, and the server can be started by executingnode dist/api/src/server.js
/api
and one for/api/src
. The root tsconfig is only responsible for type-checking which is run in precommit, and this includes the tests as well. The api/src tsconfig is for building the source code for production..js
extension likeimport something from file.js
- notfile.ts
. This is because when building the code for production tsc will change all typescript code to javascript including the file extension to .js, but won't change the import statements. So anywhere an import is requiring a file with a .ts extension, it won't exist in the production build which will make the app crash.Jira link:
Type of change
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Checklist: