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

Convert Frontend to TypeScript #635

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Convert Frontend to TypeScript #635

wants to merge 5 commits into from

Conversation

akgupta89
Copy link
Member

@akgupta89 akgupta89 commented Apr 20, 2020

Proposed changes

  • Convert project to typescript => the biggest reason behind this is it helps keep the sanity of the project as it scales. Vanilla JS doesn't do a great job with this and this makes adding features for developers much easier, as well as build time protection from writing unverified / bad code.
  • Use eslint-typescript (and associated rules)
  • Switched to barreling external and internal imports
  • Use "@paths" aliasing (doesn't work right because react-scripts is silly)
  • Add default media breakpoints

@akgupta89 akgupta89 added the DO NOT MERGE A draft or abandoned PR label Apr 20, 2020
@akgupta89 akgupta89 changed the title Conversion to TypeScript Convet Frontend to TypeScript Apr 20, 2020
@akgupta89 akgupta89 added Frontend React, JavaScript, and UI stuff and removed DO NOT MERGE A draft or abandoned PR labels Apr 20, 2020
Copy link
Member

@hathix hathix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine to me. Couple things I'd add:

  • We should follow this up quickly with PRs to add types throughout the codebase (a pain, but it'll let us reap the full benefits of TypeScript)
  • Mention TypeScript in the Readme. Ideally link to a TypeScript style guide.

Comment on lines +47 to +56
class MapSpider extends Component<Props, State> {
agency: any;

routeLayers: any[];

mapRef: any;

boundUpdate: any;

constructor(props: Props) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have to do this for all components?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, properties of classes should be predefined before you should be able to access them or set them. Also updated the README for TS. The problem is Terence has been AWOL/taking a break, Chris is hit or miss. That's why I tagged Eddy and Jesse. You can verify the JS output is the same after it transpiles, which is what really matters and should be good enough for approval. I agree the typescript updates will need to be in waves, but this gives us the skeleton and some of the changes to be successful, that's why I'm rushing getting it merged in. I don't want to waste my time fighting merge conflicts.

@hathix
Copy link
Member

hathix commented Apr 20, 2020

Would like someone from the frontend side to chime in before approving.

Copy link
Contributor

@youngj youngj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Has there been any previous discussion about this?

My initial reaction is to oppose converting the frontend to TypeScript. When I worked on a large TypeScript project before, I found myself annoyed by longer edit-test-debug cycles due to the extra compilation step after each code change, and feeling like I was always playing Simon Says with the compiler (basically, having to spend a lot of extra time getting the types right in order to avoid compilation errors that would not have occurred with JavaScript). Overall, I found writing TypeScript to be quite tedious, and less enjoyable than writing JavaScript.

I haven't really found that we have frequently encountered the drawbacks of using non-typed JavaScript yet. It seems like switching to TypeScript may address potential future issues, but in the meantime it creates new issues that everyone will have to deal with immediately.

Also, given OpenTransit's base of volunteer contributors, who often only contribute for a short time and don't have time to learn a new language just for this project, using TypeScript will make it more difficult for the large fraction of people who already know JavaScript but don't know TypeScript yet.

@EddyIonescu EddyIonescu changed the title Convet Frontend to TypeScript Convert Frontend to TypeScript Apr 23, 2020
@hathix
Copy link
Member

hathix commented Apr 23, 2020

My thoughts:

  • Agree with Jesse that TypeScript can be tedious.
  • But, especially as we scale to more than 2 cities, it's going to be very important that engineers who have never talked to each other can work on each other's code. Investing in TypeScript now, before we scale, lets us avoid a crippling loss of productivity in the future.
  • Many people join new projects precisely because they want to learn a new language. And TypeScript is quite popular these days.
  • A good IDE can help you with types in TypeScript, thus making it easier to write good code (though... I'm not much of a script coder these days! :) )

Arun, I generally agree with your proposal to do TypeScript, but can you make sure it's easy for contributors to edit, compile, and tweak quickly? Like suggesting a good IDE that improves (and doesn't hinder) productivity?

@akgupta89
Copy link
Member Author

akgupta89 commented Apr 23, 2020

Has there been any previous discussion about this?

My initial reaction is to oppose converting the frontend to TypeScript. When I worked on a large TypeScript project before, I found myself annoyed by longer edit-test-debug cycles due to the extra compilation step after each code change, and feeling like I was always playing Simon Says with the compiler (basically, having to spend a lot of extra time getting the types right in order to avoid compilation errors that would not have occurred with JavaScript). Overall, I found writing TypeScript to be quite tedious, and less enjoyable than writing JavaScript.

I haven't really found that we have frequently encountered the drawbacks of using non-typed JavaScript yet. It seems like switching to TypeScript may address potential future issues, but in the meantime it creates new issues that everyone will have to deal with immediately.

Also, given OpenTransit's base of volunteer contributors, who often only contribute for a short time and don't have time to learn a new language just for this project, using TypeScript will make it more difficult for the large fraction of people who already know JavaScript but don't know TypeScript yet.

I'm not sure how long ago you worked on a TS project, but the edit-testing-debug times are negligible at this point. You also save time on making errors by not mutating types and know the type of the argument you're passing into a function without having to read through the code and having to "guess," furthermore adding on top an IDE that adds extra benefits. Capital One has a huge frontend application built in typescript + angular and the load time for everything is about 5 seconds (just once), and subsequent file changes are loaded almost instant.

If you're extremely familiar with an application, it is easy to write code in JavaScript without the knowledge. Dealing with issues now is how you avoid technical debt, not write bad code and kick the can down the road because you don't want to face problems when they're small so you don't have to disable things and face a major refactor (e.g. isochrone has to disable 12+ linting rules). Switching to TypeScript early on is a lot easier than switching to it when its a huge codebase.

TypeScript teaches proper programming skills and how to avoid writing bad mutable code, learning types is important regardless of what language you are working on.

@EddyIonescu
Copy link
Member

What are your thoughts on Flow?

I've used it on much larger codebases than this, and at both places they chose it over TypeScript as they wanted the benefits of type-checking but since the codebases were already vanilla they didn't want the transition to decrease productivity or block ongoing projects, which is the issue that Jesse raised.

Personally Flow didn't decrease my productivity as there was no compilation step, you add the types in-line as you're coding, and it's not required everywhere (ie. you can add types only where you feel they'd be necessary) so it's easy to prototype changes and shouldn't increase the difficulty of contributing.

Also the previous frontend was in the process of transitioning to Flow (trynmaps/opentransit-map#72) before focus shifted to this repo.

I've never used TypeScript but I've heard that it aims to provide an experience similar to static typing (with the VSCode auto-completion and confidence benefits but also the productivity drawbacks) whereas Flow tries to be a type checker that doesn't get in the way.

@akgupta89
Copy link
Member Author

What are your thoughts on Flow?

I've used it on much larger codebases than this, and at both places they chose it over TypeScript as they wanted the benefits of type-checking but since the codebases were already vanilla they didn't want the transition to decrease productivity or block ongoing projects, which is the issue that Jesse raised.

Personally Flow didn't decrease my productivity as there was no compilation step, you add the types in-line as you're coding, and it's not required everywhere (ie. you can add types only where you feel they'd be necessary) so it's easy to prototype changes and shouldn't increase the difficulty of contributing.

Also the previous frontend was in the process of transitioning to Flow (trynmaps/opentransit-map#72) before focus shifted to this repo.

I've never used TypeScript but I've heard that it aims to provide an experience similar to static typing (with the VSCode auto-completion and confidence benefits but also the productivity drawbacks) whereas Flow tries to be a type checker that doesn't get in the way.

This implementation doesn't get in the way currently, it is currently not enforcing any of the typing rules. This is mostly just a lift and shift skeleton to TypeScript. I haven't used Flow myself, but from what I've heard it's basically equivalent to TS in features, but with less users and less community / dev support.

@EddyIonescu
Copy link
Member

Agreed that TypeScript is more popular but Flow's very widely used too. It's also being actively used and maintained by Facebook so it'll definitely be supported.

Exactly, that's what it's designed to be. We have a pretty good code review process that ensures that maintainable, logical, and working code is committed, which reduces the benefit of TypeScript (and of static typing). I also don't see how typing avoids writing messy mutable code (which I agree is an issue with some contributors being new to JS and pretending it's Java) but that's why we have a code review process.

I'll play around with TypeScript and with your PR next week to get a feel for it, and also look into setting up a Flow branch. What are your thoughts on trying out Flow as well? In any case I agree there should be some type checking given that the size of the project's increasing.

Also I know you want to get this out but activity's been pretty light lately so I don't think there'll be any significant conflicts in the next week or so.

@akgupta89
Copy link
Member Author

akgupta89 commented Apr 23, 2020

Agreed that TypeScript is more popular but Flow's very widely used too. It's also being actively used and maintained by Facebook so it'll definitely be supported.

Exactly, that's what it's designed to be. We have a pretty good code review process that ensures that maintainable, logical, and working code is committed, which reduces the benefit of TypeScript (and of static typing). I also don't see how typing avoids writing messy mutable code (which I agree is an issue with some contributors being new to JS and pretending it's Java) but that's why we have a code review process.

I'll play around with TypeScript and with your PR next week to get a feel for it, and also look into setting up a Flow branch. What are your thoughts on trying out Flow as well? In any case I agree there should be some type checking given that the size of the project's increasing.

Also I know you want to get this out but activity's been pretty light lately so I don't think there'll be any significant conflicts in the next week or so.

Code reviews are only as good as the weakest link, and this automates the lowest level of patterns that stop bad code from being written.

"I also don't see how typing avoids writing messy mutable code " I hope this is a typo, no pun intended haha. But, if not typing prevents mutable code from being written like so:

let x: number = 24;
x = "25";
if (x.length > 10)

Imagine being introduced to a code base and seeing this, and imagine it is abstracted away in some low level transformation function. You'd probably cringe if you had to work on this project. Some people write code like this, some people don't catch this during review, and some people don't know any better.

Flow looks pretty cool. I looked into it and it looks like its being transpiled with babel into JS, so its still a compiled language just as TypeScript. I don't know if there is a hardcore build time type checking like TypeScript, but if the type checking is done only the IDE, VSCode also does the same thing for TypeScript natively (to answer @neel) but not to the same degree. The one thing nice about TypeScript is its backed by Microsoft, Google (Angular), and even the node founder who is building Deno to kill off node (which is a Node runtime version of TypeScript). The engineer in me wants to see both of them and use the better technology, the guy keeping trying to recruit people on this project in me thinks that working on a TypeScript project is more useful for someone's resume and landing a job.

Also, I apologize to all the engineers for not reaching a consensus on this before making these changes (I wrote it on slack if you guys didn't see it). I've mentioned this migration 1 or 2 times, just never drove a consensus. But Sunday night I just had a moment of mental clarity, and I figured it was the best time to knock out this much needed high level of effort code upgrade while I was operating like that. Its just like one of those things you just KNOW is just good engineering practice and needs to be done even if people can't spot the benefits right away (like migrating from Bootstrap to Material UI, or dropping Jquery in favor of React, or lodash in favor of ES6+ features).

@EddyIonescu
Copy link
Member

EddyIonescu commented Apr 23, 2020

@akgupta89 that's true, I meant that TypeScript doesn't stop people from defining such an object, although at least then it's typed so everyone knows about it so I see your point (I considered bringing ImmutableJS at one point but it felt like too much). And it would shorten the review process which is a good thing too (requiring Flow types would have a similar effect though).

Flow can be run in the command-line and as part of a build process, but it's really always running as you're typing so there are plug-ins for VSCode and other editors for error messages and auto-completion. Also it is transpiled, which just removes the annotations, but doesn't require changing file extensions.

Yeah TypeScript is a bigger buzzword than Flow but our goal is to get members to ship something (so they get an open-source contribution to show off which even better), so Flow might have an edge there. Thanks for getting this started - I still think Flow's worth looking into but I'll also get back to this PR and trying out TypeScript, sorry I don't have time this week but I definitely will after this Wednesday.

@akgupta89
Copy link
Member Author

@akgupta89 that's true, I meant that TypeScript doesn't stop people from defining such an object, although at least then it's typed so everyone knows about it so I see your point (I considered bringing ImmutableJS at one point but it felt like too much). And it would shorten the review process which is a good thing too (requiring Flow types would have a similar effect though).

Flow can be run in the command-line and as part of a build process, but it's really always running as you're typing so there are plug-ins for VSCode and other editors for error messages and auto-completion. Also it is transpiled, which just removes the annotations, but doesn't require changing file extensions.

Yeah TypeScript is a bigger buzzword than Flow but our goal is to get members to ship something (so they get an open-source contribution to show off which even better), so Flow might have an edge there. Thanks for getting this started - I still think Flow's worth looking into but I'll also get back to this PR and trying out TypeScript, sorry I don't have time this week but I definitely will after this Wednesday.

Ah, that was my bad, I was speaking more broadly about variable type mutations. It sounds to me like the only difference is Flow doesn't require changing extensions. Since you have experience with Flow, it might be an easier of a comparison to download this branch and open it in VSCode (or your favorite IDE with a TS plugin), and if there are major benefits to gain by switching to Flow vs TS.

I think the selling point of a project like this is we're using all the most popular technologies, which is a good recruiting tool given that working on this project: you will develop the skills needed to be successful in a real world job where these skills are all in high demand. Which is another way to increase development velocity. And a lot of people already know TypeScript, much like React. If I was to pitch this to someone they would learn Vue, even when some could argue Vue improves on React, it's still not much of a selling point unless you're simply interested in the "latest and greatest." Another benefit to TS is that basically all the top libraries support it already, so typings are defined when you're importing an external library (like Material). And at this point, since typings are mostly optional in this project, it doesn't hold back anything except preventing people from writing code that doesn't actually work or is being improperly accessed when typings exist. If you look closely at the file changes, it pretty much caught issues of accessing properties that don't exist and also defining the structure of objects (which can be turned off).

Regardless, if you feel like Flow improves on this on a cost/benefit analysis level and is worth pursuing, we should be all look at it. I'm gonna switch back to working on making a strategy on a migration to a serverless frontend build again. Hopefully I'll get a discussion going on the tradeoffs before I write code this time around :P

@hathix
Copy link
Member

hathix commented Apr 30, 2020

Hey all, let's not get too into the weeds here. I think the main questions are:

  • Which language (TS or Flow) has better support and documentation?
  • Which language are more people likely to know or use in their day jobs?

If one language clearly wins on both, let's just go with it.

@akgupta89
Copy link
Member Author

Hey all, let's not get too into the weeds here. I think the main questions are:

  • Which language (TS or Flow) has better support and documentation?
  • Which language are more people likely to know or use in their day jobs?

If one language clearly wins on both, let's just go with it.

TS wins on both counts, and the demand is growing for that skill too. Also worth noting I'm not sure Flow offers anything TS doesn't already, TS is designed to work in mixed, optional, and TS only codebases depending on your needs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Frontend React, JavaScript, and UI stuff
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants