search

nextstrain / nextstrain.org

The Nextstrain website
https://nextstrain.org
GNU Affero General Public License v3.0
87 stars 49 forks source link

Start using TypeScript #871

Open victorlin opened 1 month ago

victorlin commented 1 month ago

It would be nice to allow usage of TypeScript files in this project similar to how it was introduced in Auspice: https://github.com/nextstrain/auspice/pull/1450

This would affect the way the site is built, but given the benefits of TypeScript, I think we're all fine with the added complexity (mentioned briefly in https://github.com/nextstrain/private/issues/88)

Tasks

Notes:

  1. "configure" means allow the ability to use .ts/.tsx files with curated options in tsconfig.json. Actual usage of TypeScript is expected to happen incrementally over time.
  2. We should create separate issues for the long-term items. Listing them here first for clarity and decision making purposes.
genehack commented 1 month ago

big flaming +1

jameshadfield commented 1 month ago

One salient difference between this project and Auspice is that Auspice has a bundler/transpiler layer whereas this repo runs directly through nodejs. In other words there's no "build stage" for the server.

Is this issue proposing using ts-node (or similar), or proposing adding a build stage, or something else?

victorlin commented 1 month ago

there's no "build stage" for the server

Previously there wasn't, but now there is with the addition of Next.js, at least based on my understanding in https://github.com/nextstrain/nextstrain.org/pull/810#discussion_r1558304436.

I just noticed 2eedca4d7eba4b88012eb63fa694f9d44f2fff1b already introduced traces of TypeScript: a tsconfig file and 2 TSX files. So it looks like it's already used behind the scenes by Next.js - I think we just need to make our own configurations to use it elsewhere.

jameshadfield commented 1 month ago

Previously there wasn't, but now there is with the addition of Next.js

Not quite -- the frontend still has a build stage (edit: in production mode only, in dev mode it's compiled on-demand as pages are requested), but that's directly analogous to the previous set up where gatsby also had a build stage. With nextjs this now uses TS, and since it's no longer siloed into static-site/ quite like it used to be we have the top-level tsconfigs. However the server is still run directly though node -- underneath it all we're just running node server.js. To use TS this would have to change. There are many ways of making this change -- ts-node, tsc ... && node ..., build/transpilation/bundling/chaos etc. (I'm all for moving to TS, but I want to point out we are adding complexity here and should tread carefully.)

(Further complicating the layers involved here, nextjs running from within our server actually runs in its own (nodejs?) process and I suspect it's able to use TS files somehow, but AFAICS that doesn't help us introduce TS into our server. Lots of layers.)

victorlin commented 1 month ago

I see – I forgot that there is still a big distinction between frontend and server even though they share the same package.json now. Yeah it would be worth trying a few options and comparing the complexities here - I haven't used any of them before.

since it's no longer siloed into static-site/ quite like it used to be we have the top-level tsconfigs

It's still under static-site/tsconfig.json and can't be moved up to top-level with the way the site is built currently.

genehack commented 1 month ago

I see – I forgot that there is still a big distinction between frontend and server even though they share the same package.json now.

FWIW, $JOB[-1] had a monorepo with an Express-based backend app / API server and a Svelte-based front-end. They shared a package.json (IIRC anyway) and there wasn't a perceptively painful build step for the backend during normal development, only when we built a specific artifact for deployment (to AWS, as a "lambda-lith").

Happy to mock up a small demo repo with what that looked like if folks would like to kick the tires. (Note: this is not a proposal to convert the front-end to Svelte!)

jameshadfield commented 1 month ago

This would affect the way the site is built, but given the benefits of TypeScript

Upon re-reading this today I realise there may be two separate conversations going on in this thread (and that may be on me!)

Adding TS to the (nodejs) frontend The frontend (static-site/) already uses TS, albeit only for a few files (e.g.). Type errors cause build failures, and thus CI fails. I think the TS config used is taken directly from the nextjs defaults, but we can totally change this as needed. Gradually moving more frontend files to TS seems a great idea.

Adding TS to the server This is what my comments in previous messages were focused on. It would (probably) introduce an extra layer, but as long as we picked well this wouldn't be too burdensome. Having worked in the server a bunch recently I would strongly support gradual introduction of TS. As part of this we could also move the resource indexer to TS in the same fashion.

The above paints these as separate siloed projects, which is essentially true at the moment. If we moved to SSR and shared information between our server and frontend pages (e.g. as I did in this prototype, and Tom's done elsewhere as well) then it's a little more complicated as we'd want to share type definitions between the silos.

genehack commented 1 month ago

The above paints these as separate siloed projects, which is essentially true at the moment. If we moved to SSR and shared information between our server and frontend pages (e.g. as I did in this prototype, and Tom's done elsewhere as well) then it's a little more complicated as we'd want to share type definitions between the silos.

I'd urge doing that as a deliberate two-step (or three-step?) process: get everything using Typescript, get a good start on adding typing everywhere, and only then worry about refactoring out a set of common shared types.

victorlin commented 1 month ago

I realise there may be two separate conversations going on in this thread (and that may be on me!)

Great to split these out - thanks for making the distinction. I've updated the issue description to reflect this.