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

Lf 4395 setup typescript on backend #3428

Open
wants to merge 29 commits into
base: integration
Choose a base branch
from

Conversation

navDhammu
Copy link
Collaborator

@navDhammu navDhammu commented Sep 11, 2024

This sets up typescript on the backend. New and existing files in src and tests 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

  • Requires node version >=20.6. This is enforced using the engines option in package.json and engine-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.
  • For development - npm run dev replaces npm run nodemon. Nodemon is not needed as node.js comes with watch mode after node v18
  • For production - the source code is compiled to /dist via tsc, and the server can be started by executing node dist/api/src/server.js
  • Babel is replaced with swc
  • There are 2 separate tsconfigs, one for root dir /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.
  • When importing typescript files, you need to import them with the .js extension like import something from file.js - not file.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

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

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

  • Passes test case
  • UI components visually reviewed on desktop view
  • UI components visually reviewed on mobile view
  • Other (please explain)

Checklist:

  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • The precommit and linting ran successfully
  • I have added or updated language tags for text that's part of the UI
  • I have added "MISSING" for all new language tags to languages I don't speak
  • I have added the GNU General Public License to all new files

@navDhammu navDhammu force-pushed the LF-4395-setup-typescript-on-backend branch from 714f667 to 84f3923 Compare September 29, 2024 15:12
@navDhammu navDhammu marked this pull request as ready for review September 30, 2024 16:41
@navDhammu navDhammu requested review from a team as code owners September 30, 2024 16:41
@navDhammu navDhammu requested review from antsgar and Duncan-Brain and removed request for a team September 30, 2024 16:41
@navDhammu navDhammu added the enhancement New feature or request label Sep 30, 2024
@antsgar
Copy link
Collaborator

antsgar commented Oct 2, 2024

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.

@navDhammu
Copy link
Collaborator Author

@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 npm run dev to keep the same functionality? But if its meant for production then this wouldn't be ideal and we should be using a production command instead.

@Duncan-Brain
Copy link
Collaborator

Duncan-Brain commented Oct 9, 2024

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 engine and engine-strict are cool! We have definitely had people on the wrong version before causing bugs so that is cool! Also awesome that swc reduces test time, that is super handy because its like 13mins on CI! Only other replacement for nodemon I have tried is tsx which is pretty easy to work with.

I tested and everything worked well on:

  • dev script
  • migrations on the test database

I tested and it fails:

  • local docker compose file - specifically the export service on the node engine flag

I expect to fail

  • deployment - due to prod.Dockerfile

So my other current thoughts are:

  1. Node version - totally agree with @antsgar that it would be good to update this across packages if we are updating (I might gently suggest a prequel PR for that and also Typescript version -- see below)

    • one reason for this is I think most devs use the local commands instead of docker and manually changing node version when working between packages could be troublesome
    • Files I saw it needs updating version were, beta_export_deploy.sh and prod.Dockerfile
  2. Nodemon - yes it is used, yes it shouldn't be - but we do need the command to work in the prod.Dockerfile line. As long as it does then we can punt the bad nodemon use issue and discuss on a different ticket.

  3. Typescript version - Same as node version it might be good to update across packages if we are updating

    • I saw a lot of TS errors on your branch because my vscode settings uses Litefarms ./node_modules/typescript/lib as the Intellisense parser (they are gone when I manually switch ts source) - and for dev experience it would be great to have them all match so that we don't have to manually switch across packages to see others errors
    • This could maybe even be set in our codebase even under the /.vscode/settings.json
  4. Tsconfig files

    • There is two - If there is any way we can have just one by using command inline flags like --noEmit or other I think that could be good
    • It's interesting that there is a package for a base tsconfig - curious about the value/risks vs copy pasting and attribution?
  5. Import syntax and .js vs .ts.

@navDhammu
Copy link
Collaborator Author

@Duncan-Brain

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 init.sh and dev.export.sh using nodemon which also might need to be changed. I'm thinking in dev.export.sh, the line npx nodemon --inspect=0.0.0.0:9229 src/jobs/index.js can just be replaced with node --inspect=0.0.0.0:9229 src/jobs/index.js? And for init.sh it can be replaced with npm run dev?

Export server failing in docker compose:

  • To fix this the node version needs to be updated in the litefarm/node-awscli:0.0.2 image, I checked by running docker inspect litefarm/node-awscli:0.0.2 and its using v18.

Typescript version:

  • We can update it across all packages but since we are using a monorepo I think a better way would be to use a single installation of typescript in the root and share that across all packages. This way it's installed in just one place rather than duplicated across multiple node_modules, and vscode settings can be pointed to that installation.

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.

@Duncan-Brain
Copy link
Collaborator

Duncan-Brain commented Oct 15, 2024

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

npx and npm i -g implementations of nodemon should still work since they are not part of package.json-- so we can put the scope of that onto a different ticket I think. I think only thing in prod.Dockerfile I was sure wouldn't work was the old node version. nodemon start line itself.. might have worked.

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 build for beta/production. So that would be done in a dockerfile and we would select the folders we copy to the image -- which would exclude tests and other things not used on prod.

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 .ts . Or even using extensionless (which might be handier actually.. at least for me my vscode autocompletes import, it assumes I am using react and does not add the extension to the import which results in errors :( haha)

@navDhammu
Copy link
Collaborator Author

navDhammu commented Oct 15, 2024

@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:

  • I don't think some of those existing scripts with nodemon will work even though they're used with npx. For example if you look at npx nodemon --inspect=0.0.0.0:9229 src/jobs/index.js in the export script, if any typescript files are being imported by src/jobs/index.js then it'll probably crash since the typescript wouldn't have been compiled. Or it might work for now because there's no typescript (except for server.ts), but once you start to add new ts files it'll eventually break.

Tsconfig:

  • It might be possible to keep one tsconfig that way with docker, but will have to test it out.
  • To answer your other question about using an external package for the base tsconfig - I thought its good to have a set of base recommended options based on the node version we're using. Ofcourse we can just copy paste it but then whenever we change versions, we have to look it up and copy paste again - its easier to just update the package with npm.

For typescript version:

  • I was wondering if I should just make the temporary change in this PR and update typescript across all packages?
  • Or if we want to go for a long term solution, I think something like pnpm workspaces would be ideal because they have great built-in support for monorepos and we're already using pnpm in webapp, so we just need to update the api and other packges to use pnpm, and this isn't just useful for typescript but other common dependencies like prettier, eslint, etc. I did a quick search in jira and found this old ticket for migrating to pnpm workspaces. Any thoughts on this?

@Duncan-Brain
Copy link
Collaborator

Node version 👍

Nodemon -- right it would need to have the build command first then it would change to something using the /dist right? like npx nodemon --inspect=0.0.0.0:9229 dist/src/jobs/index.js

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 npm lol... based on a suggestion by LiteFarm Alumni Jimmy he shared this - https://x.com/ruyadorno/status/1621186886470361093?t=qQbp9OVEKTBpdmWkmpGBOQ&s=19 either way probabaly out of scope for the typescript for backend improvement

@navDhammu
Copy link
Collaborator Author

navDhammu commented Oct 16, 2024

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 import something from 'package-x' even if you don't have package-x listed as dependency, simply because package-x was installed as a dependency of some other dependency.

@navDhammu
Copy link
Collaborator Author

@Duncan-Brain @antsgar

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants