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

Make the TypeScript types strict and better #758

Closed
7 of 9 tasks
sindresorhus opened this issue Mar 21, 2019 · 29 comments · Fixed by #1278
Closed
7 of 9 tasks

Make the TypeScript types strict and better #758

sindresorhus opened this issue Mar 21, 2019 · 29 comments · Fixed by #1278
Labels
enhancement This change will extend Got features 🎁 Rewarded on Issuehunt This issue has been rewarded on Issuehunt ✭ help wanted ✭

Comments

@sindresorhus
Copy link
Owner

sindresorhus commented Mar 21, 2019

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):


IssueHunt Summary

pmmmwh pmmmwh has been rewarded.

Backers (Total: $180.00)

Submitted pull Requests


Tips

@sindresorhus sindresorhus added enhancement This change will extend Got features ✭ help wanted ✭ labels Mar 21, 2019
@sindresorhus sindresorhus added this to the v10 milestone Mar 21, 2019
@paulmelnikow
Copy link
Contributor

The work I did at #700 (comment) 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?

@IssueHuntBot
Copy link

@IssueHunt has funded $180.00 to this issue.


@sindresorhus
Copy link
Owner Author

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

@paulmelnikow
Copy link
Contributor

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

@sindresorhus
Copy link
Owner Author

Yes, definitely.

@paulmelnikow
Copy link
Contributor

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 added a commit to paulmelnikow/got that referenced this issue Mar 28, 2019
This removes `noUnusedParameters: false`, one of the tasks in the checklist at sindresorhus#758.

After reading these documents, it seems like either `_` or e.g. `_unusedRequest` is a good replacement name which silences the errors for these parameters

- microsoft/TypeScript#24249
- microsoft/TypeScript#9458
@sindresorhus
Copy link
Owner Author

sindresorhus commented Mar 28, 2019

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

@paulmelnikow
Copy link
Contributor

Ah, I gotcha. Thanks!

@tobenna
Copy link

tobenna commented Apr 5, 2019

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

master...tobenna:strict-ts

@mastermatt
Copy link
Contributor

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?

@sindresorhus
Copy link
Owner Author

@mastermatt Yes

@sindresorhus sindresorhus mentioned this issue Apr 18, 2019
3 tasks
@AxelTerizaki

This comment has been minimized.

@sindresorhus

This comment has been minimized.

@ejmartin504
Copy link

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?

@sindresorhus
Copy link
Owner Author

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

@vladfrangu
Copy link
Contributor

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? 😄

@yakov116
Copy link

yakov116 commented May 3, 2019

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

@issuehunt-oss issuehunt-oss bot added the 💵 Funded on Issuehunt The issue has been funded on Issuehunt label May 10, 2019
@sindresorhus
Copy link
Owner Author

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

@sindresorhus
Copy link
Owner Author

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

@vladfrangu
Copy link
Contributor

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!

@sindresorhus
Copy link
Owner Author

Sounds good. Thank you :)

@sindresorhus
Copy link
Owner Author

This is still up for grabs.

@pmmmwh
Copy link
Contributor

pmmmwh commented Nov 9, 2019

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?

@sindresorhus
Copy link
Owner Author

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.

@sindresorhus
Copy link
Owner Author

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.

@pmmmwh
Copy link
Contributor

pmmmwh commented Dec 17, 2019

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.

@sindresorhus
Copy link
Owner Author

@pmmmwh Still insterested in finishing this?

@pmmmwh
Copy link
Contributor

pmmmwh commented May 21, 2020

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

@issuehunt-oss
Copy link

issuehunt-oss bot commented Aug 16, 2020

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

  • 💰 Total deposit: $180.00
  • 🎉 Repository reward(0%): $0.00
  • 🔧 Service fee(10%): $18.00

@issuehunt-oss issuehunt-oss bot added 🎁 Rewarded on Issuehunt This issue has been rewarded on Issuehunt and removed 💵 Funded on Issuehunt The issue has been funded on Issuehunt labels Aug 16, 2020
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 🎁 Rewarded on Issuehunt This issue has been rewarded on Issuehunt ✭ help wanted ✭
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants