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

Improve types #946

Merged
merged 24 commits into from Dec 1, 2019
Merged

Improve types #946

merged 24 commits into from Dec 1, 2019

Conversation

pmmmwh
Copy link
Contributor

@pmmmwh pmmmwh commented Nov 26, 2019

This is a follow-up to #928 . Hopefully this is the last batch of (type breaking?) changes 🤞 .

Changes

  • Remove force casting when appropriate (some are left cause they are intended to be errors)
  • Better exported types so that users wouldn't have to force cast
  • Lint project to fully comply with XO rules
  • All TODO comments related to types should have been resolved by Make TypeScript types conforms to strict mode #928 or this PR
  • All fixable ts-ignore comments should have been removed by Make TypeScript types conforms to strict mode #928 or this PR
  • All non-fixable ts-ignore comments should have a description stating what happened there
  • Refactor/Write/Copy-n-Paste Doc-String for exported types

Checklist

  • I have read the documentation.
  • I have included a pull request description of my changes.

@sindresorhus
Copy link
Owner

sindresorhus commented Nov 27, 2019

Did you see #876 (comment) ?

DefinitelyTyped/DefinitelyTyped#34960

@pmmmwh
Copy link
Contributor Author

pmmmwh commented Nov 27, 2019

Did you see #876 (comment) ?

DefinitelyTyped/DefinitelyTyped#34960

I actually just read that. Should we just augment the global scope and add the two classes there? I don't see another way out ...

@sindresorhus
Copy link
Owner

I actually just read that. Should we just augment the global scope and add the two classes there? I don't see another way out ...

Yeah, we can do that for now.

@pmmmwh
Copy link
Contributor Author

pmmmwh commented Nov 28, 2019

Pushed a commit to finish up the work on ts-ignore comments. Afaik most of the work related to types (other than doc strings) should be done?

@sindresorhus sindresorhus changed the title Follow up on Types Improve types Nov 29, 2019
@sindresorhus
Copy link
Owner

One more that could use a better type:

const piped = new Set<any>(); // TODO: Should be `new Set<stream.Writable>();`.

And maybe also:

// @ts-ignore FIXME: Incompatible union type signatures

@sindresorhus
Copy link
Owner

Regarding the doc strings. I think that should be a separate PR after this one. I'd like to get this one merged asap so we can do the final Got v10 release.

source/as-promise.ts Outdated Show resolved Hide resolved
source/utils/types.ts Outdated Show resolved Hide resolved
source/utils/types.ts Outdated Show resolved Hide resolved
@szmarczak
Copy link
Collaborator

Also I've fixed some types here: https://github.com/sindresorhus/got/pull/833/files#diff-05c76183bbee7e002aa6711e6baca2ef so you may want to take a look and fix it here also.

@szmarczak
Copy link
Collaborator

LGTM but it'd be nice if you merged my fixes into this PR, I don't expect my PR to get merged before yours.

@pmmmwh
Copy link
Contributor Author

pmmmwh commented Nov 29, 2019

One more that could use a better type:

const piped = new Set<any>(); // TODO: Should be `new Set<stream.Writable>();`.

And maybe also:

// @ts-ignore FIXME: Incompatible union type signatures

The first one - I think the interface is actually 100% equal to the ServerResponse class from the 'http' library? I checked stream.Writable and it didn't seem to have a good fit of properties.

The second one - I haven't found a way to work around that. It seems to be a TypeScript problem as stated here? I can try removing the generics for CancelableRequest and ProxyStream (as suggested as a cause by someone in the thread) and see if it fixes that.

source/as-promise.ts Outdated Show resolved Hide resolved
source/as-stream.ts Outdated Show resolved Hide resolved
source/as-stream.ts Outdated Show resolved Hide resolved
source/as-stream.ts Outdated Show resolved Hide resolved
@pmmmwh
Copy link
Contributor Author

pmmmwh commented Nov 29, 2019

LGTM but it'd be nice if you merged my fixes into this PR, I don't expect my PR to get merged before yours.

@szmarczak Do you want me to pull in all changes related to types from there? I am particularly not very confident on pulling in changes in this file, since there is some change in normalization logic too?

@szmarczak
Copy link
Collaborator

I'll send a PR to your PR (types-follow-up branch) ;)

@szmarczak
Copy link
Collaborator

@pmmmwh Sent you a PR ;)

@szmarczak
Copy link
Collaborator

If you've got any questions, feel free to ask. This is great work! :D

@pmmmwh
Copy link
Contributor Author

pmmmwh commented Nov 30, 2019

If you've got any questions, feel free to ask. This is great work! :D

Checked and merged 😺

@sindresorhus sindresorhus merged commit 71b8452 into sindresorhus:master Dec 1, 2019
@sindresorhus
Copy link
Owner

Awesome! Thanks for all your work on this, @pmmmwh! We could not have done this without you. 🙌

@pmmmwh pmmmwh deleted the types-follow-up branch December 6, 2019 15:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants