Got: Make the TypeScript types strict and better

Created on 21 Mar 2019  ·  29Comments  ·  Source: sindresorhus/got


Issuehunt badges

We have successfully transitioned to TypeScript, but we have not yet enabled strict mode. Although some of the files have strict types, there's still a lot of work to properly type everything.

If you want to take on this issue, it's expected that you have deep TypeScript understanding and experience. It's not an easy issue. Comment if you decide to start on this to prevent duplicate work.

I suggest the following order of fixing things (can be multiple PRs):

  • [x] Remove "strict": false, in tsconfig.json and properly type what's required just to get it to compile ($ npm test should pass).
  • [x] Improve existing typing and ensure everything has as specific and correct types are possible. (Some progress has been made: https://github.com/sindresorhus/got/pull/760)
  • [x] Remove noImplicitReturns from tsconfig.json and fix any errors.
  • [x] Remove noUnusedParameters from tsconfig.json and fix any errors.
  • [x] Remove these lines in package.json and fix errors.
  • [x] Fix type-related TODO comments.
  • [x] Remove // @ts-ignore comments and fix the errors.
  • [ ] Add doc comments to public (meaning exported) functions/types/interfaces. Borrow text from readme. This especially applies to the options and the main Got interface. Follow this style: https://github.com/sindresorhus/typescript-definition-style-guide#documentation
  • [ ] Use readonly and const assertions wherever possible: https://devblogs.microsoft.com/typescript/announcing-typescript-3-4/




IssueHunt Summary

pmmmwh pmmmwh has been rewarded.

Backers (Total: $180.00)

Submitted pull Requests

Tips


Rewarded on Issuehunt enhancement ✭ help wanted ✭

Most helpful comment

Add doc comments to public (meaning exported) functions/types/interfaces. Borrow text from readme. This especially applies to the options and the main Got interface. Follow this style: sindresorhus/typescript-definition-style-guide#documentation

@pmmmwh I think ⬆️ is the only thing left to resolve this issue.

I'm starting to work on this. I'll ask if I hit any road blockers, but I don't think there will be much.

All 29 comments

The work I did at https://github.com/sindresorhus/got/issues/700#issuecomment-466831401 will need to be rebased. I will probably need some pointers about how to resolve some of the remaining issues. Is that something you'd be able to help with / is it worth doing that?

@issuehunt has funded $180.00 to this issue.


@paulmelnikow I'm not sure how much my review will help. Making Got strict was more over my current TS knowledge than I realized.

Would it be helpful to incrementally add the parts for which I _was_ able to identify good types?

Yes, definitely.

Out of curiosity, why did you remove noImplicitReturns from the top post instead of checking it off? It's nice to feel like progress has been made. Also the second bullet isn't done, but could reference #760 which made significant progress.

@paulmelnikow It was only half of the bullet point, so was easier to remove it than to move it to a new one and check it, but done now.

Ah, I gotcha. Thanks!

I'm working on:

Remove "strict": false, in tsconfig.json and properly type what's required just to get it to compile ($ npm test should pass).

https://github.com/sindresorhus/got/compare/master...tobenna:strict-ts

I understand the TS migration is still a wip (big fan), but at this time do consumers of this lib still need to use @types/got?

@mastermatt Yes

I'm not sure if I should open a new issue or post it here, so here goes :

I'm transitionning my own app to Typescript and installed @types/got

However, I'm running into a TS error with this bit of code :

const res = await got.stream(`https://${instance}/avatars/${avatarFile}`, {
        stream: true
    });
const avatarPath = resolve(getState().appPath, conf.System.Path.Temp, avatarFile);
await writeStreamToFile(res, avatarPath);

I'm not sure if there's a better way to use streams and promises, but in this case it worked just fine before I transitionned to Typescript. Now, I get the following error :

Argument of type '{ stream: boolean; }' is not assignable to parameter of type 'GotOptions'.
Object literal may only specify known properties, and 'stream' does not exist in type 'GotOptions'.

Hope this helps in making types better for Got... or if someone knows a workaround/solution because I did something wrong, I'm all ears.

I'm transitionning my own app to Typescript and installed @types/got

We don't control @types/got. You need to open an issue on https://github.com/DefinitelyTyped/DefinitelyTyped

await got.stream is also moot as await a non-promise does nothing. Remove the await.

I would love to help out with this issue, even though I'm a bit of a TS n00b. What would be the best bullet point to work on?

@ejmartin504 You could remove the "strict": false, config temporarily, fix/improve some types, and then put "strict": false, back again. We also need help adding doc comments (taken from the readme).

Hello!

I'd like to help out with this issue, with making the project strict and fulfill the list. What do I need to do to get started? 😄

@vladfrangu the comment above yours

You could remove the "strict": false, config temporarily, fix/improve some types, and then put "strict": false, back again. We also need help adding doc comments (taken from the readme).

To prevent any duplicate work, I just want to make it clear that @vladfrangu is currently working on this issue :)

@vladfrangu Have you had the chance to do some more work on it?

I haven't really had a lot of free time recently, which sucks a lot, and I'm so sorry about that! I'll check out what needs to be done when I get back in around 2 weeks to get the project to compile with strict mode (on the latest TS version too). I'll keep you up to date as much as I can!

Sounds good. Thank you :)

This is still up for grabs.

This is still up for grabs.

Hey! I can work on this issue next week. I guess a good first PR would be to make the source code strict mode compatible?

I guess a good first PR would be to make the source code strict mode compatible?

Yes, just keep in mind that is harder than it looks. There have been multiple attempts already, which have improved the types a lot, but still not managed to make it 100% strict.

Add doc comments to public (meaning exported) functions/types/interfaces. Borrow text from readme. This especially applies to the options and the main Got interface. Follow this style: sindresorhus/typescript-definition-style-guide#documentation

@pmmmwh I think ⬆️ is the only thing left to resolve this issue.

Add doc comments to public (meaning exported) functions/types/interfaces. Borrow text from readme. This especially applies to the options and the main Got interface. Follow this style: sindresorhus/typescript-definition-style-guide#documentation

@pmmmwh I think ⬆️ is the only thing left to resolve this issue.

I'm starting to work on this. I'll ask if I hit any road blockers, but I don't think there will be much.

@pmmmwh Still insterested in finishing this?

@pmmmwh Still interested in finishing this?

Yes - sorry that I've been quite busy with other stuff for the past few months.

I've opened a draft PR (#1278) to track progress.

@sindresorhus has rewarded $162.00 to @pmmmwh. See it on IssueHunt

  • :moneybag: Total deposit: $180.00
  • :tada: Repository reward(0%): $0.00
  • :wrench: Service fee(10%): $18.00
Was this page helpful?
0 / 5 - 0 ratings

Related issues

joolfe picture joolfe  ·  3Comments

quocnguyen picture quocnguyen  ·  4Comments

dominusmars picture dominusmars  ·  3Comments

khizarsonu picture khizarsonu  ·  3Comments

tkoelpin picture tkoelpin  ·  3Comments