compdemocracy / polis

:milky_way: Open Source AI for large scale open ended feedback
https://pol.is
GNU Affero General Public License v3.0
782 stars 186 forks source link

RFC: Gradual TypeScript Adoption for Current JavaScript codebase #960

Closed micahstubbs closed 2 years ago

micahstubbs commented 3 years ago

Problem: Reliability could be better. Deployments break for hard to debug reasons. Interfaces are less documented than we'd like, and sometimes unclear to new contributors.

Suggested solution: Adopt TypeScript gradually for the current JavaScript codebase.

Adopting TypeScript is not a binary choice, you can start by annotating existing JavaScript with JSDoc, then switch a few files to be checked by TypeScript and over time prepare your codebase to convert completely.

TypeScript’s type inference means that you don’t have to annotate your code until you want more safety.

Additional context: https://www.typescriptlang.org/

micahstubbs commented 3 years ago

@colinmegill I think you had mentioned that if we pursue this, we could start in a specific, more self contained part of the codebase. Was that the admin panel?

micahstubbs commented 3 years ago

@patcon I'd also like to hear your thoughts generally about this proposal, as well as what part of the code base would be a good place to start.

patcon commented 3 years ago

This seems like a great idea! Never worked with typescript, but heard great things.

Will this make any attempt at merging admin+participation+report (esp handlebones stuff) any harder or easier, or can they mix and play nice and one area of code is unconcerned that others are in typescript? :)

micahstubbs commented 3 years ago

I'd also like to try to identify and hard dependencies that we would need to start transpiling new .ts or .tsx TypeScript to JavaScript.

Do we need to make changes to the existing gulp config?

Do we need to migrate to webpack, in part or in whole?

Would it make sense to adopt @patcon's razzle PR #515 , to quickly enable TypeScript transpiliation? (Ideally these too things would be separate, but I could imagine how it could make sense to do them together.)

patcon commented 3 years ago

Not sure which intuitions to trust, but I can see it being easier for admin (since already really nice and reacty), but maybe bigger gains for participation (since it needs so much love). Leaning toward participation getting typescript attention first, just bc its the only experience of so many ppl, and improving that DX allows the user-facing bits to move more fearlessly and responsively :)

Again, no practical experiences with TS though, so take my hot-takes with grain of salt 😁

micahstubbs commented 3 years ago

Will this make any attempt at merging admin+participation+report (esp handlebones stuff) any harder or easier, or can they mix and play nice and one area of code is unconcerned that others are in typescript? :)

Easier if all are typed.

The same if not, as you can just type the interactions with untyped code as any to start with.

patcon commented 3 years ago

Do we need to make changes to the existing gulp config? Do we need to migrate to webpack, in part or in whole?

My sense is that, yeah, build system (gulp=>webpack) might need attention before moving to TS. Stuck on a few diff versions of node until then, it seems

micahstubbs commented 3 years ago

makes sense. I'll checkout a branch and try to get TypeScript tooling setup.

@patcon if you are interested in doing the same experiment, I am happy to review and merge your TS config if you get it working before I do.

colinmegill commented 3 years ago

Thanks a ton @micahstubbs and @patcon! An aside, for performance reasons, client-participation is a separate bundle intentionally. client-report and client-admin will be merged, and client-report modernized along the way.

There are a lot of dead routes on the server, and a number of routes that could be:

  1. cleaned up of comments
  2. updated to ES6 const / etc
  3. updated to async / await instead of .catch
  4. reconsider the idea of .p.

https://github.com/compdemocracy/polis/blob/dev/server/src/server.js#L7670

micahstubbs commented 3 years ago

okay, I've checked out a branch to add typescript to the server directory sub-project

WIP PR here https://github.com/compdemocracy/polis/pull/961/files

patcon commented 2 years ago

Do we want to close this? Or we could add a checklist to the body of the initial comment to track all the steps for typescript conversion?

EDIT: And if closing, do we spin out tasks from @colinmegill's comment above: https://github.com/compdemocracy/polis/issues/960#issuecomment-820028778

colinmegill commented 2 years ago

Closing as we've made progress. As a next discussion topic, we would want to create a list of high priority types to add to https://github.com/compdemocracy/polis/blob/dev/server/src/d.ts in our next pass at typing