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

Rewrite Got in TypeScript #700

Closed
sindresorhus opened this issue Jan 13, 2019 · 28 comments
Closed

Rewrite Got in TypeScript #700

sindresorhus opened this issue Jan 13, 2019 · 28 comments
Labels
enhancement This change will extend Got features ✭ help wanted ✭
Milestone

Comments

@sindresorhus
Copy link
Owner

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.

@sindresorhus sindresorhus added the enhancement This change will extend Got features label Jan 13, 2019
@gajus

This comment has been minimized.

@szmarczak

This comment has been minimized.

@sindresorhus

This comment has been minimized.

@gajus

This comment has been minimized.

@sindresorhus

This comment has been minimized.

@gajus

This comment has been minimized.

@ferdaber
Copy link
Contributor

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

@sindresorhus sindresorhus added this to the v10 milestone Jan 17, 2019
@sindresorhus
Copy link
Owner Author

@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.

@medikoo

This comment has been minimized.

@sindresorhus

This comment has been minimized.

@medikoo

This comment has been minimized.

@medikoo

This comment has been minimized.

@sindresorhus

This comment has been minimized.

@sindresorhus

This comment has been minimized.

@sindresorhus
Copy link
Owner Author

sindresorhus commented Jan 17, 2019

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').

@ricardorojass
Copy link
Contributor

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

@michaeldera
Copy link

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

@sindresorhus
Copy link
Owner Author

@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
Copy link

@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

@Magellol
Copy link
Contributor

Magellol commented Feb 2, 2019

Started to tackle https://github.com/sindresorhus/got/blob/master/source/as-stream.js in #720 👍

@michaeldera
Copy link

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

@sindresorhus
Copy link
Owner Author

sindresorhus commented Feb 22, 2019

We are still looking for help getting this done. 🙌

Left to do:

  • 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.

@ghost
Copy link

ghost commented Feb 22, 2019

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.

@tobenna
Copy link

tobenna commented Feb 22, 2019

I can do source/normalize-arguments.js

@sindresorhus
Copy link
Owner Author

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.

@sindresorhus
Copy link
Owner Author

@tobenna 👍 Great

@paulmelnikow
Copy link
Contributor

paulmelnikow commented Feb 24, 2019

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

Added: Making progress: master...paulmelnikow:request-as-event-emitter-ts

@sindresorhus
Copy link
Owner Author

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 #758

Thanks for all the help everyone. ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement This change will extend Got features ✭ help wanted ✭
Projects
None yet
Development

No branches or pull requests

10 participants