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

Migrate code base to TypeScript #335

Closed
parisk opened this issue Nov 2, 2016 · 9 comments
Closed

Migrate code base to TypeScript #335

parisk opened this issue Nov 2, 2016 · 9 comments
Assignees
Labels
type/proposal A proposal that needs some discussion before proceeding
Milestone

Comments

@parisk
Copy link
Contributor

parisk commented Nov 2, 2016

One of the things proposed in order to increase the quality of this code base as it grows (both in code and popularity) was migrate from JavaScript to TypeScript.

TypeScript sports several cool features like type safety, classes etc. and is a superset of JavaScript, so not much will change.

Questions

There are some questions though that have to be answered before making such a "big" move:

  • What problems that we have already ran into will be solved by TypeScript's features (e.g. type safety, classes etc.)?
  • Timing: Does it need to be implemented now (why?) or can wait until we really need this?
  • What are possible regressions that might be introduced by this?
  • What should we change in our current toolset?

Concerns

My main concerns about moving code base to TypeScript are:

  1. Not sure if this will be a barrier to entry for new contributors
  2. Not all maintainers are familiar with TypeScripts (so most of us will need to learn a new language and adapt to its ecosystem)
  3. Might be an overkill for this project

/cc @Tyriar since IMO you can contribute better into this.

@parisk parisk added the type/proposal A proposal that needs some discussion before proceeding label Nov 2, 2016
@blink1073
Copy link
Contributor

Hi @parisk, a few comments from the peanut gallery based on experience in https://github.com/jupyterlab/jupyterlab.

  1. We have had contributors who were new to JavaScript, as well as ones who had used JavaScript heavily before. The use of TypeScript made it much easier as maintainers to have confidence in PRs from new contributors. Having types also forces a minimal set of documentation about arguments and return types, that technically could be in JSDoc but is prone to forgetfulness or getting out of sync. Most of the reservations we heard from the wider community were difficulty in getting a new project in TypeScript set up, which is a lot easier with TypeScript 2.0. Other complaints included how to get typings for dependencies, which is again mitigated in TypeScript 2.0.

  2. You can use as much or as little of the language as you like, depending on your use of compiler settings, and can gradually opt in to things like null checking.

  3. Once you have the build system set up, it overall is a decrease in cognitive burden for both maintainers and new contributors, and is a great aid when refactoring.

@Tyriar
Copy link
Member

Tyriar commented Nov 2, 2016

What problems that we have already ran into will be solved by TypeScript's features (e.g. type safety, classes etc.)?

  • The biggest problem for me that I think this solves is to help with refactoring by making the code less fragile. There's quite a large portion of xterm.js which is very difficult to touch and essentially all PRs need to be tested manually to ensure quality. I'm having difficulty starting with Allow xterm.js to exist and receive/send data before it's attached to the DOM #266 for example which is essentially a bit refactor.
  • Tests are no longer using a combination of src/xterm, dist/xterm and build/xterm but are instead compiled with the source code for tests and then left out of the dist build.

Timing: Does it need to be implemented now (why?) or can wait until we really need this?

Why not now? I believe this will boost productivity so earlier is better.

What are possible regressions that might be introduced by this?

It shouldn't introduce any regressions apart from the the shuffling of addons (whose locations aren't technically part of the API). It could identify some obscure bugs however when we start the process of adding the typings.

What should we change in our current toolset?

-babel +typescript

Not sure if this will be a barrier to entry for new contributors

A colleague looked into this when determining whether to use JS or TS for their team's new project and came back saying there may be a few contributors that will not contribute but the benefits you gain as mentioned above outweigh the loss of this small portion of contributors. As I mention below it's more users who refuse to use it, instead of trying, as the syntax is ES6 with some self-explanatory niceties added on.

Not all maintainers are familiar with TypeScripts (so most of us will need to learn a new language and adapt to its ecosystem)

When I started on VS Code I didn't know TypeScript either but never the less felt productive right from the start. Most of the syntax is based on ES6 or self explanatory with the exception of typing which is a huge benefit to quality (plus also self-explanatory). The only real difficulty I've faced since picking up TypeScript is setting up a project with it.

Might be an overkill for this project

xterm.js is currently sitting at over 5000 lines and it's not the only file, that's not a particularly small library.

@akalipetis
Copy link
Contributor

Although I'm not one of the main contributors to Xterm.js, I'd like to share my two cents here too.

TLDR: I believe that switching to TypeScript would be the way to go, I just have a concern.

How is is it to gradually switch our code? Are there any best practices of doing such a switch? How big the "minimum" effort needed for the first TypeScript release?

Both @Tyriar experience with TypeScript and @blink1073 with doing such a switch in the past would be of great value here.

On the bright side now:

  1. Having types really helps in code quality and understanding, while also having a compiler catch errors is definitely a plus!
  2. Though my experience is minimal, I liked the way TypeScript works and I see many frameworks adopting it, ie you can use Angular, React and others in TypeScript which is shows good adoption by the community

@blink1073
Copy link
Contributor

We went the wholesale path of using the "noImplicitAny": true compiler flag, changing a file to .ts, and making the compiler happy. I'd recommend this over the more gradual approach of simply converting the file to .ts without that flag and slowly adding types. It immediately puts you in the mindset of making sure everything is explicitly typed. When you are about to reach for an explicit any type, you probably wanted a union type (e.g. number | boolean | 'foo'), or an interface.

@akalipetis
Copy link
Contributor

I understand, but I believe that using this flag will make the xterm.js -> xterm.ts change a hell of a ride, right?

I'm not sure we'll be ready to go this way. Unless we gradually extract pieces from the core to .ts files...

@blink1073
Copy link
Contributor

In my experience even larger files tend to go rather quickly once you get into the mode of adding types. Most of the types are self-evident.

@akalipetis
Copy link
Contributor

👍 thanks a lot for the insights. I'll take a deeper look given the above info.

@Tyriar
Copy link
Member

Tyriar commented Nov 4, 2016

I was planning on doing the majority of the conversion, the plan was to convert the existing modules first (CompositionHelper, Viewport, etc.) and then tackle xterm.js by either doing a once over to get it into a reasonable state or factoring out more modules at that point if that's easier.

Tyriar added a commit to Tyriar/xterm.js that referenced this issue Nov 21, 2016
@Tyriar Tyriar changed the title Examine migration of code base to TypeScript Migrate code base to TypeScript Nov 22, 2016
Tyriar added a commit to Tyriar/xterm.js that referenced this issue Nov 22, 2016
parisk added a commit that referenced this issue Dec 6, 2016
Also apply JavaScript rules in `.editorconfig` in TypeScript files as well.

Part of #335
parisk added a commit that referenced this issue Dec 6, 2016
Also apply JavaScript rules in `.editorconfig` in TypeScript files as well.

Part of #335
parisk added a commit that referenced this issue Dec 6, 2016
Also apply JavaScript rules in `.editorconfig` in TypeScript files as well.

Part of #335
Tyriar added a commit to Tyriar/xterm.js that referenced this issue Dec 29, 2016
Tyriar added a commit to Tyriar/xterm.js that referenced this issue Dec 31, 2016
Tyriar added a commit to Tyriar/xterm.js that referenced this issue Jan 3, 2017
Tyriar added a commit to Tyriar/xterm.js that referenced this issue Jan 19, 2017
@parisk parisk modified the milestone: 3.0.0 Aug 3, 2017
@parisk parisk mentioned this issue Aug 3, 2017
13 tasks
@Tyriar Tyriar self-assigned this Aug 7, 2017
@Tyriar
Copy link
Member

Tyriar commented Aug 7, 2017

The core lib is done with #839. Addons moved to #840. Remaining tests moved to #862.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/proposal A proposal that needs some discussion before proceeding
Projects
None yet
Development

No branches or pull requests

4 participants