Got: Rewrite Got in TypeScript

Created on 13 Jan 2019  ·  28Comments  ·  Source: sindresorhus/got

We want typings available and keeping a type definition file in sync with the actual codebase is a lot of work in itself and it's easy for them to get out of sync. I also think the codebase could benefit from being written in TypeScript, as the code is quite complex and it's easy to make subtle mistakes.

enhancement ✭ help wanted ✭

Most helpful comment

The TS setup is done, so now it's possible to convert to TS file-by-file. If you want to contribute, pick one or more files you think you can manage and have fun. Just rename the .js file to .ts and try to build ($ npm run build). Make sure you read https://github.com/sindresorhus/typescript-definition-style-guide for the style we use and document all public interfaces (you can steal text from the readme) (bonus points for documenting private ones too). Also try not to do files that are touched a lot in the open PRs, to avoid conflicts.

Note: To import a TS file into a JS file, you need to use require('foo').default instead of require('foo').

All 28 comments

What is the reason for choosing TS of Flow?

if I were to rewrite Got, I would do it in TypeScript, not Flow, as that's what I'm familiar with and prefer.

TypeScript is just more popular and @sindresorhus has had experience with TypeScript for a long time, it's easier to maintain so.

It's mainly about familiarity and popularity. Popularity means it's easier to attract potential contributors. But I've also noticed that TS has better error messages, and the fact that TS is written in TS (meaning JS) is definitely a bonus. Very few knows OCaml.

@sindresorhus Do you have an example of a specific error that TS produces that is more developer friendly than what Flow would produce?

The other two arguments make sense. OCaml codebase indeed makes it for a steep entry barrier to contribute to the codebase, and TS has gained a lot of popularity over the last 6 months.

Do you have an example of a specific error that TS produces that is more developer friendly than what Flow would produce?

It's a common sentiment (there are multiple popular blog posts mentioning this, but can't find them right now), not just my experience. I have dabbled in Flow too and experienced hard to understand errors.

That comment is one year old.

Flow errors improved _a lot_ over the last 12 months.

Now errors show where the issue originates and walks you through all incompatibilities down the graph.

screenshot 2019-01-14 at 10 37 43

This helps a lot with refactoring.

I am biased of course as I hardly use TS. Flow feels like a second skin.

Either way, I think that TS is the right choice for Got for the other reasons you have suggested. I was more curious what is driving the decision rather than influencing the decision.

This looks like an interesting project- I'd like to help! I can start with at least adding the TypeScript build/test steps.

@ferdaber Yes, that would be helpful. See https://github.com/sindresorhus/is for how to do the TS setup. Should use https://github.com/sindresorhus/tsconfig and https://github.com/xojs/tslint-xo.

Aren't you worried that complexity it'll add, will shadow the benefit?

got codebase is now pretty straightforward to get through, and library works very well (doesn't feel buggy in any part).

What real problem converting to TS aims to solve?

@medikoo The codebase is already quite complex. Adding types will let us be more confident about doing changes. Another big benefit is that we don't have to maintain a separate TS type definition file, which could easily get out of sync.

Another big benefit is that we don't have to maintain a separate TS type definition file, which could easily get out of sync.

Can't this be solved simply with JSDoc? I haven't explored that deeply, but I think TS can read types of out it as well

Adding types will let us be more confident about doing changes

Do you remember any specific issues that wouldn't happen if it'll be TS already?

This move also makes more difficult to contribute for JS developers not interested or not familiar with TS. It narrows the audience.

Can't this be solved simply with JSDoc? I haven't explored that deeply, but I think TS can read types of out it as well

No, JSDoc helps to some degree, but TS is much more powerful.

This is also not really the place to discuss the merits of TS. We have already decided to use it. I would recommend asking in my AMA instead.

Do you remember any specific issues that wouldn't happen if it'll be TS already?

Not specifically, but I remember some instances where we shipped bugs because of unpredicted conditions that would have been caught with stronger types. It's not just about catching problems either, but it makes it easier for us to work on Got. We now get auto-completion for everything and can do large refactors with ease.

This move also makes more difficult to contribute for JS developers not interested or not familiar with TS. It narrows the audience.

I'm ok with that. By requiring TS I think we'll get higher quality contributions too, as it sets a higher minimum JS/TS knowledge level. In reality though, this is irrelevant as 99.9% of the commit in Got are by the maintainers.

The TS setup is done, so now it's possible to convert to TS file-by-file. If you want to contribute, pick one or more files you think you can manage and have fun. Just rename the .js file to .ts and try to build ($ npm run build). Make sure you read https://github.com/sindresorhus/typescript-definition-style-guide for the style we use and document all public interfaces (you can steal text from the readme) (bonus points for documenting private ones too). Also try not to do files that are touched a lot in the open PRs, to avoid conflicts.

Note: To import a TS file into a JS file, you need to use require('foo').default instead of require('foo').

Hi everyone, I'd like to help with this project, I'll be going to rewrite source/errors.js in TS.

I would love to help rewrite the files in TypeScript, is there a specific file I can go for?

@michaeldera Pick any .js file you think you can handle and that's not already being worked on in an open PR ;)

For example: https://github.com/sindresorhus/got/blob/master/source/create.js

@michaeldera Pick any .js file you think you can handle and that's not already being worked on in an open PR ;)

For example: https://github.com/sindresorhus/got/blob/master/source/create.js

Now working on create.js

721 converted create.js to TypeScript. Ran into one issue and could use help with that. I added the details in the PR

We are still looking for help getting this done. 🙌

Left to do:

  • [x] source/as-promise.js
  • [ ] source/normalize-arguments.js
  • [ ] source/request-as-event-emitter.js
  • [ ] source/index.js (the above files should be done first)

When these are done, we'll need help converting the tests to TypeScript.

fwiw and feel free to disagree but since a lot of people will consume this package in a JS project (meaning I'm just making this assumption) then tests should probably remain in JS. Consumers calling from JS into TS do not get the benefit of static type safety and accordingly the Got APIs should not make any assumptions about validity of arguments passed in.

If the tests are written in TS and say an API function takes a number (and is typed as such) then when one tries to write a test that passes in, say, a string instead of a number then TS will produce a compile error. To work around the difficulty of writing tests to check the robustness of APIs against any arbitrary types of values thrown at them, one ends up doing a lot of any cast gymnastics.

I can do source/normalize-arguments.js

To work around the difficulty of writing tests to check the robustness of APIs against any arbitrary types of values thrown at them, one ends up doing a lot of any cast gymnastics.

I've been doing that in other TS-written modules. It's not a big problem to write a few as any.

@tobenna 👍 Great

I will give source/request-as-event-emitter.js a shot!

Added: Making progress: https://github.com/sindresorhus/got/compare/master...paulmelnikow:request-as-event-emitter-ts

I decided to disable strict-mode for now and move the rest of the files over to TS so the TS migration would not be blocking other work. The rest of the work is moved into https://github.com/sindresorhus/got/issues/758

Thanks for all the help everyone. ❤️

Was this page helpful?
0 / 5 - 0 ratings

Related issues

sindresorhus picture sindresorhus  ·  3Comments

alanzhaonys picture alanzhaonys  ·  4Comments

darksabrefr picture darksabrefr  ·  3Comments

joolfe picture joolfe  ·  3Comments

khizarsonu picture khizarsonu  ·  3Comments