-
-
Notifications
You must be signed in to change notification settings - Fork 935
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 TypeScript types conforms to strict mode #928
Make TypeScript types conforms to strict mode #928
Conversation
Beware of #921 - if it gets merged you'll have to rework many many things :P I'd prefer if you waited for its merge. |
test/create.ts
Outdated
@@ -27,7 +32,7 @@ test('supports instance defaults', withServer, async (t, server, got) => { | |||
'user-agent': 'custom-ua-string' | |||
} | |||
}); | |||
const headers = await instance('').json(); | |||
const headers = await instance('').json() as Headers; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const headers = await instance('').json() as Headers; | |
const headers = await instance('').json<Headers>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This couldn't be done because TS complains Headers
is not assignable to JsonValue
. I am still thinking of a way to solve that ...
I tried both ways below with no luck:
const headers = await instance('').json<Headers>();
const headers: Headers = await instance('').json();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like I've commented elsewhere, don't use JsonValue
. It creates more problems than it solves.
Generally, don't use as
unless absolutely needed, as it can hide problems, since it just force casts anything to anything.
I start to hate TypeScript. |
Okay, with a lot of trial and errors I made tests run on Node 10/12 on Travis. I used I'll probably figure out both tomorrow. |
I saw @sindresorhus tweet about it. TS is still short when to infer/track types when we are in a highly-mutable, very functional (with recursions) world, or when there are a lot of overloads to the same function. It does offer a lot of promise, but when a lot of stuff is unknown (especially true for request body), it is so tempting to just go with |
source/utils/types.ts
Outdated
@@ -117,7 +117,7 @@ export interface Delays { | |||
request?: number; | |||
} | |||
|
|||
export type Headers = Record<string, string | string[] | undefined>; | |||
export type Headers = Record<string, string | string[] | number | undefined>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should support number
here. Headers are strings, so I think it's better to force users to explicitly coerce/cast to a string themselves.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is referenced from the test for content-length: 0
. Should I alter the test?
Edit: I altered the test to toString()
a number.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
test/create.ts
Outdated
@@ -27,7 +32,7 @@ test('supports instance defaults', withServer, async (t, server, got) => { | |||
'user-agent': 'custom-ua-string' | |||
} | |||
}); | |||
const headers = await instance('').json(); | |||
const headers = await instance('').json() as Headers; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like I've commented elsewhere, don't use JsonValue
. It creates more problems than it solves.
Generally, don't use as
unless absolutely needed, as it can hide problems, since it just force casts anything to anything.
type OptionsOfTextResponseBody = Options & {isStream?: false; resolveBodyOnly?: false; responseType: 'text'}; | ||
type OptionsOfJSONResponseBody = Options & {isStream?: false; resolveBodyOnly?: false; responseType: 'json'}; | ||
type OptionsOfBufferResponseBody = Options & {isStream?: false; resolveBodyOnly?: false; responseType: 'buffer'}; | ||
const isGotInstance = (value: any): value is Got => ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const isGotInstance = (value: any): value is Got => ( | |
const isGotInstance = (value: unknown): value is Got => ( |
@@ -217,13 +224,13 @@ export const normalizeArguments = (url: URLOrOptions, options?: Options, default | |||
if (is.urlInstance(url) || is.string(url)) { | |||
options.url = url; | |||
|
|||
options = mergeOptions(defaults && defaults.options, options); | |||
options = mergeOptions((defaults && defaults.options) ?? {}, options); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
options = mergeOptions((defaults && defaults.options) ?? {}, options); | |
options = mergeOptions(defaults?.options ?? {}, options); |
} else { | ||
if (Reflect.has(url, 'resolve')) { | ||
throw new Error('The legacy `url.Url` is deprecated. Use `URL` instead.'); | ||
} | ||
|
||
options = mergeOptions(defaults && defaults.options, url, options); | ||
options = mergeOptions((defaults && defaults.options) ?? {}, url, options); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
options = mergeOptions((defaults && defaults.options) ?? {}, url, options); | |
options = mergeOptions(defaults?.options ?? {}, url, options); |
@@ -29,7 +30,8 @@ const prepareServer = server => { | |||
}); | |||
}; | |||
|
|||
const createAgentSpy = Cls => { | |||
// TODO: Type this stricter if possible | |||
const createAgentSpy = <T extends any>(Cls: T) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const createAgentSpy = <T extends any>(Cls: T) => { | |
const createAgentSpy = <T>(Cls: T) => { |
And let's use a clearer name than Cls
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. unknown
doesn't seems to work here because it has to be something with a construct-able signature. Since unknown
is not callable, it can never fulfill the type requirements?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, but it should at least be something more specific than any
then.
@@ -14,7 +15,7 @@ test('reads a cookie', withServer, async (t, server, got) => { | |||
|
|||
const cookieJar = new toughCookie.CookieJar(); | |||
|
|||
await got({cookieJar}); | |||
await got({cookieJar} as unknown as OptionsOfDefaultResponseBody); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this need force-casting?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did this because I hit an issue where TypeScript is always using the first overload of the CookieJar's methods to match the use cases in our source code (we defined a custom interface), which is actually using the third overload. Is there a way to make a proper comparison happen?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overloads are order-dependent, so maybe try changing the order? If this is in fact a TS issue, it would be good to try to find an issue on the TS issue tracker and add a code comment linking to it.
result.modified = true; | ||
|
||
return result; | ||
} | ||
] | ||
] as HandlerFunction[] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this need force-casting?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be resolved, it was my oversight.
Edit: Hmm. Not really. It is complaining CancellableRequest
!== Promise
(an async function must return Promise<T>
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not clear to me what is complaining, but users should not need to force cast this.
@@ -68,7 +70,7 @@ test('retry function gets iteration count', withServer, async (t, server, got) = | |||
retry: { | |||
calculateDelay: ({attemptCount}) => { | |||
t.true(is.number(attemptCount)); | |||
return attemptCount < 2; | |||
return Number(attemptCount < 2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this needed? attemptCount
is not documented to ever be a different value or undefined
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From reading the source it seems that the return value of calculateDelay
would be consumed by computedValue
, which expects a number. When we return attemptCount < 2
, it is returning a boolean
type, and to cast it to a number would need Number()
, or use attemptCount < 2 ? 1 : 0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
attemptCount < 2 ? 1 : 0
would be much clearer. Can you update https://github.com/sindresorhus/got#testing too?
declare module 'create-test-server' { | ||
import {Express} from 'express'; | ||
|
||
function createTestServer(options: any): Promise<createTestServer.TestServer>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
function createTestServer(options: any): Promise<createTestServer.TestServer>; | |
function createTestServer(options: unknown): Promise<createTestServer.TestServer>; |
|
||
namespace createTestServer { | ||
export interface TestServer extends Express { | ||
caCert: any; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
caCert: any; | |
caCert: unknown; |
There are still a lot of force-casting ( |
@pmmmwh I'm merging this now as tests are passing, to reduce potential merge conflicts for you and so that we can get this tested out in a beta version I plan to release now. Can you continue the remaining work in a new PR? |
Just wanted to say that you've done an amazing job so far 🙌✨ |
Sure! I will continue the work tomorrow probably - thanks for merging this! |
This PR tries to make all code within the
source
folder conforms to strict mode.There are some parts that I had to trace the source code and make educated guesses as I am uncertain with the actual available values, so please do tell me if I am wrong! Also - there are some places we could either use an intermediate variable or use non-null assertions. Please tell me which one you would prefer to maintain consistency 👀
For #758
Next Steps
const
assertions that can be appliedChecklist
IssueHunt Summary
Referenced issues
This pull request has been submitted to:
IssueHunt has been backed by the following sponsors. Become a sponsor