Make TypeScript types conforms to strict mode #928

pmmmwh · 2019-11-13T19:08:24Z

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

Confirm source code is running fine
Fix types in tests
Fix ts-ignore and typescript eslint errors
Check for potential const assertions that can be applied

Checklist

I have read the documentation.
I have included a pull request description of my changes.
I have included some tests.
If it's a new feature, I have included documentation updates.

IssueHunt Summary

Referenced issues

This pull request has been submitted to:

#758: Make the TypeScript types strict and better

IssueHunt has been backed by the following sponsors. Become a sponsor

szmarczak · 2019-11-14T19:30:43Z

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.

szmarczak · 2019-11-23T20:55:38Z

test/create.ts

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


Suggested change

const headers = await instance('').json() as Headers;

const headers = await instance('').json<Headers>();

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();

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.

szmarczak · 2019-11-23T20:57:51Z

I start to hate TypeScript.

pmmmwh · 2019-11-23T21:32:22Z

Okay, with a lot of trial and errors I made tests run on Node 10/12 on Travis.
One test failed. I checked the history and I'm pretty sure that I didn't touch that part of the code.

I used JsonValue to type JSON responses, but it seems to have some side-effect and will complain any object like interface !== JsonValue.

I'll probably figure out both tomorrow.

pmmmwh · 2019-11-23T21:37:47Z

I start to hate TypeScript.

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

source/as-promise.ts

source/create.ts

sindresorhus · 2019-11-24T04:19:17Z

source/utils/types.ts

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


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.

This is referenced from the test for content-length: 0. Should I alter the test?

Edit: I altered the test to toString() a number.

source/utils/types.ts

test/cache.ts

sindresorhus · 2019-11-24T04:22:04Z

test/create.ts

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


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.

test/create.ts

test/hooks.ts

test/progress.ts

source/request-as-event-emitter.ts

# Conflicts: # test/progress.ts

sindresorhus · 2019-11-24T05:22:46Z

source/create.ts

-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 => (


Suggested change

const isGotInstance = (value: any): value is Got => (

const isGotInstance = (value: unknown): value is Got => (

sindresorhus · 2019-11-24T05:24:39Z

source/normalize-arguments.ts

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


Suggested change

options = mergeOptions((defaults && defaults.options) ?? {}, options);

options = mergeOptions(defaults?.options ?? {}, options);

sindresorhus · 2019-11-24T05:24:54Z

source/normalize-arguments.ts

 	} 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);


Suggested change

options = mergeOptions((defaults && defaults.options) ?? {}, url, options);

options = mergeOptions(defaults?.options ?? {}, url, options);

sindresorhus · 2019-11-24T05:29:22Z

test/agent.ts

@@ -29,7 +30,8 @@ const prepareServer = server => {
 	});
 };

-const createAgentSpy = Cls => {
+// TODO: Type this stricter if possible
+const createAgentSpy = <T extends any>(Cls: T) => {


Suggested change

const createAgentSpy = <T extends any>(Cls: T) => {

const createAgentSpy = <T>(Cls: T) => {

And let's use a clearer name than Cls.

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?

Ah, but it should at least be something more specific than any then.

test/arguments.ts

sindresorhus · 2019-11-24T05:32:49Z

test/cookies.ts

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


Why does this need force-casting?

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?

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.

sindresorhus · 2019-11-24T05:33:41Z

test/create.ts

 				result.modified = true;

 				return result;
 			}
-		]
+		] as HandlerFunction[]


Why does this need force-casting?

This can be resolved, it was my oversight.

Edit: Hmm. Not really. It is complaining CancellableRequest !== Promise (an async function must return Promise<T>).

Not clear to me what is complaining, but users should not need to force cast this.

sindresorhus · 2019-11-24T05:35:25Z

test/retry.ts

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


Why is this needed? attemptCount is not documented to ever be a different value or undefined.

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

attemptCount < 2 ? 1 : 0 would be much clearer. Can you update https://github.com/sindresorhus/got#testing too?

sindresorhus · 2019-11-24T05:35:58Z

test/types/create-test-server/index.d.ts

+declare module 'create-test-server' {
+	import {Express} from 'express';
+
+	function createTestServer(options: any): Promise<createTestServer.TestServer>;


Suggested change

function createTestServer(options: any): Promise<createTestServer.TestServer>;

function createTestServer(options: unknown): Promise<createTestServer.TestServer>;

sindresorhus · 2019-11-24T05:36:08Z

test/types/create-test-server/index.d.ts

+
+	namespace createTestServer {
+		export interface TestServer extends Express {
+			caCert: any;


Suggested change

caCert: any;

caCert: unknown;

sindresorhus · 2019-11-24T05:36:56Z

There are still a lot of force-casting (as) in the tests that should ideally not be there.

…ce-strict-types

sindresorhus · 2019-11-25T04:39:15Z

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

sindresorhus · 2019-11-25T04:41:16Z

Just wanted to say that you've done an amazing job so far 🙌✨

pmmmwh · 2019-11-25T12:28:51Z

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

Sure! I will continue the work tomorrow probably - thanks for merging this!

pmmmwh added 15 commits November 14, 2019 00:12

Add declaration for Reflect.has for type assertion

eb38ead

Add DeepPartial utility type

e4339f0

Refactor library types for default values

701a738

Fix utils to be strict mode compatible

f150894

Fix error types

9fb4dd1

Fix type errors related to options merging

e56471d

Fix type errors for event emitter

eb10198

Fix type errors for retry calculation

e93c3ff

General type cleanup on source code

db48597

Re-order dependencies and enable strict mode

02e2e50

Use global URL instead of imported URL

08af93a

Use global URLSearchParams instead of import

2552a0f

Linting

2ec0170

Scope variables for Cbs instead of using non-null assertion

333817d

Fix dumb code applied during testing

85e6dcf

pmmmwh changed the title ~~Make TypeScript types conforms to strict mode~~ [WIP] Make TypeScript types conforms to strict mode Nov 13, 2019

pmmmwh added 12 commits November 14, 2019 03:46

Linting - prefer destructuring

0c51020

Fix typo on defaults and mismatch of object and record

858ec60

Revert changes on errors

85446b7

Fix mistakenly moved lines

2332037

Further enhance types on created instance based on tests

9864fd5

Update types for created got instance and migrate to use type-fest

83f5030

Allow unknown hooks to be injected

bd847ae

Linting with XO

9d3709b

Add types for create-test-server and slow-stream

4f9f6a1

Cleanup promise return types and instance types

614a553

Fix hooks types to be more true

7660d44

Type with server test helper

94abad7

Extend with server to allow non promise usage

984381a

szmarczak reviewed Nov 23, 2019

View reviewed changes

pmmmwh added 5 commits November 24, 2019 05:06

Update error types

35d960a

Better type as-promise

a4344e0

Better type as-stream

ce32120

Fix spacing in package.json

0a65461

Cleanup types for options

d0c9de7

sindresorhus requested changes Nov 24, 2019

View reviewed changes

pmmmwh added 5 commits November 24, 2019 13:04

Address PR comments and keep types passing

ec930bf

Merge remote-tracking branch 'upstream/master' into enforce-strict-types

279972c

# Conflicts: # test/progress.ts

Cleanup for default response as string type

13b05b7

Update Record with any to object syntax

71c9f6d

Fix linting error

dc7f6ba

sindresorhus requested changes Nov 24, 2019

View reviewed changes

sindresorhus and others added 6 commits November 24, 2019 12:37

Update tsconfig.json

95f222a

Fix typo in normalizing arguments

d7a40c5

Fix tests

3177779

Merge remote-tracking branch 'origin/enforce-strict-types' into enfor…

fca92b9

…ce-strict-types

Merge branch 'master' into enforce-strict-types

7d68f73

Update normalize-arguments.ts

4cd6e02

sindresorhus merged commit c537dee into sindresorhus:master Nov 25, 2019

pmmmwh deleted the enforce-strict-types branch November 26, 2019 20:29

pmmmwh added a commit to pmmmwh/got that referenced this pull request Nov 26, 2019

Address PR comments in sindresorhus#928 and fix force casting in source

5999af4

pmmmwh mentioned this pull request Nov 26, 2019

Improve types #946

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make TypeScript types conforms to strict mode #928

Make TypeScript types conforms to strict mode #928

pmmmwh commented Nov 13, 2019 •

edited by issuehunt-oss bot

szmarczak commented Nov 14, 2019

szmarczak Nov 23, 2019

pmmmwh Nov 23, 2019

sindresorhus Nov 24, 2019 •

edited

szmarczak commented Nov 23, 2019

pmmmwh commented Nov 23, 2019 •

edited

pmmmwh commented Nov 23, 2019

sindresorhus Nov 24, 2019

pmmmwh Nov 24, 2019 •

edited

sindresorhus Nov 24, 2019

sindresorhus Nov 24, 2019 •

edited

sindresorhus Nov 24, 2019

sindresorhus Nov 24, 2019

sindresorhus Nov 24, 2019

sindresorhus Nov 24, 2019

pmmmwh Nov 24, 2019

sindresorhus Nov 24, 2019

sindresorhus Nov 24, 2019

pmmmwh Nov 24, 2019 •

edited

sindresorhus Nov 24, 2019

sindresorhus Nov 24, 2019

pmmmwh Nov 24, 2019 •

edited

sindresorhus Nov 24, 2019

sindresorhus Nov 24, 2019

pmmmwh Nov 24, 2019

sindresorhus Nov 24, 2019

sindresorhus Nov 24, 2019

sindresorhus Nov 24, 2019

sindresorhus commented Nov 24, 2019

sindresorhus commented Nov 25, 2019

sindresorhus commented Nov 25, 2019 •

edited

pmmmwh commented Nov 25, 2019

	const headers = await instance('').json() as Headers;
	const headers = await instance('').json<Headers>();

	const isGotInstance = (value: any): value is Got => (
	const isGotInstance = (value: unknown): value is Got => (

	options = mergeOptions((defaults && defaults.options) ?? {}, options);
	options = mergeOptions(defaults?.options ?? {}, options);

	const createAgentSpy = <T extends any>(Cls: T) => {
	const createAgentSpy = <T>(Cls: T) => {

	function createTestServer(options: any): Promise<createTestServer.TestServer>;
	function createTestServer(options: unknown): Promise<createTestServer.TestServer>;

Make TypeScript types conforms to strict mode #928

Make TypeScript types conforms to strict mode #928

Conversation

pmmmwh commented Nov 13, 2019 • edited by issuehunt-oss bot

Next Steps

Checklist

Referenced issues

szmarczak commented Nov 14, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sindresorhus Nov 24, 2019 • edited

Choose a reason for hiding this comment

szmarczak commented Nov 23, 2019

pmmmwh commented Nov 23, 2019 • edited

pmmmwh commented Nov 23, 2019

Choose a reason for hiding this comment

pmmmwh Nov 24, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sindresorhus Nov 24, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmmmwh Nov 24, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmmmwh Nov 24, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sindresorhus commented Nov 24, 2019

sindresorhus commented Nov 25, 2019

sindresorhus commented Nov 25, 2019 • edited

pmmmwh commented Nov 25, 2019

pmmmwh commented Nov 13, 2019 •

edited by issuehunt-oss bot

sindresorhus Nov 24, 2019 •

edited

pmmmwh commented Nov 23, 2019 •

edited

pmmmwh Nov 24, 2019 •

edited

sindresorhus Nov 24, 2019 •

edited

pmmmwh Nov 24, 2019 •

edited

pmmmwh Nov 24, 2019 •

edited

sindresorhus commented Nov 25, 2019 •

edited