Rewrite Got #1051

szmarczak · 2020-02-06T18:54:00Z

Changelog

Bug fixes

Fixes Got doesn't pass query-string when calling via unix-socket #1036 (tested)
Fixes beforeRequest hook is not called before the actual request #994 (tested)
Fixes got.stream() swallows errors when used to upload small files with stream.pipeline() #1026 (tested)
Fixes Using got.post with both body and cache options causes an exception #1021 (tested)
Fixes Got doesn't throw on leading slashes #1057 (tested)
Fixes Got throws if passed options are already frozen #1050 (tested)
Fixes TypeScript cannot figure out which Got type to use #954 (tested)
Fixes mergeOptions isn't merging URLSearchParams instances #1011 (tested)
Fixes HTTP authentication leak in redirects #1090 (tested)
Fixes Pagination should ignore resolveBodyOnly option #1140 (tested)
Fixes Cannot reuse an options object #1118 (tested)

Breaking changes

API

options.encoding will no longer affect streams (reverting back to the Got 9 behavior), use got.stream(...).setEncoding(encoding) instead (to prevent confusion)
renamed GotError to RequestError
options.agent can no longer be an instance of http.Agent - instead you need to pass:

{
	http: new http.Agent(...),
	https: new https.Agent(...),
	http2: new http2wrapper.Agent(...)
}

options.useElectronNet has been removed
dnsCache is now enabled by default
dnsCache no longer can be a Map or a Keyv instance, instead you need to pass a CacheableLookup instance
errors thrown in init hooks will be converted to RequestErrors (why: RequestError provides much more useful information (e.g. Got options) than the usual TypeError etc.)
options._pagination has been renamed to options.pagination
the options.url property will no longer be visible in init hooks (design limitation)
the error.request property is no longer a ClientRequest instance - instead, it gives a Got stream

Types

renamed the Defaults type to InstanceDefaults
renamed the DefaultOptions type to Defaults
renamed the DefaultRetryOptions type to RequiredRetryOptions
renamed the GotOptions type to Options
the ResponseObject type has been merged into the Response type
renamed the GotRequestMethod type to GotRequestFunction

Enhancements

Fixes The merge function is slow #1016 (benchmarked)
Fixes Use error.code instead of error.message to compare errors #981 (refactored)
Fixes The retry logic should be moved into as-promise.ts #932 (refactored)
Fixes Pass error thrown in the init hook to beforeError hook #929 (tested)
Fixes A way to customize cacheable-lookup #1058 (tested)
Fixes HTTP2 support #167 (tested, but there's only 1 test as of now, blocked by Use fastify in tests instead of express #1093)
Fixes Errors have undefined body when using streams #1138 (tested)
Fixes Spaces should be normalized as + in query strings #1113 (tested)
Fixes Modify response headers while using got.stream(...) #1129 (tested)
The errors now have a request property (it's a Got stream).

Miscellaneous

Fixes got actually need nodejs >= 10.9, not >= 10 #1130

Known bugs

The timings indicate that the request was successful (even though it errored).
The downloadProgress object may show incorrect data if the request has errored.

TODO

FEEL FREE TO TEST THIS

Note that this has ~~yet un~~documented breaking changes, ~~but~~ everything should work as expected - all tests pass. TypeScript recommended.

git clone https://github.com/szmarczak/got.git
git checkout rework
npm install
npm run build

const got = require('./dist/source');

(async () => {
	console.log(await got('https://httpbin.org/anything'));
})();

IssueHunt Summary

Referenced issues

This pull request has been submitted to:

#167: HTTP2 support

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

sindresorhus · 2020-02-06T19:57:39Z

I got chills down my spine when I read the PR title 😅

I hope you plan to add lots of tests. I'm very paranoid that we'll end up breaking a lot of stuff that we're not currently testing.

szmarczak · 2020-02-06T20:10:47Z

I hope you plan to add lots of tests.

Yup. Every fix & every change in behavior is gonna be tested.

I'm very paranoid that we'll end up breaking a lot of stuff that we're not currently testing.

Don't worry! There's gonna be a very detailed changelog. I'll explain why things are done like that and not in another way.

szmarczak · 2020-03-02T17:01:36Z

benchmark/index.ts

+// got - stream                           x 4,313 ops/sec ±5.61% (72 runs sampled)
+// got - promise core                     x 6,756 ops/sec ±5.32% (80 runs sampled)
+// got - stream core                      x 6,863 ops/sec ±4.68% (76 runs sampled)
+// got - stream core - normalized options x 7,960 ops/sec ±3.83% (81 runs sampled)


Normalizing cuts out ~1k req/s but the time is needed to duplicate the options, so the original ones won't be altered if you use hooks to modify the request. Hooks are our main adventage while being compared to other HTTP clients.

szmarczak · 2020-03-02T17:03:46Z

benchmark/index.ts

+// got - promise                          x 3,092 ops/sec ±5.25% (73 runs sampled)
+// got - stream                           x 4,313 ops/sec ±5.61% (72 runs sampled)


I don't know why, but the got function cuts out 3k-4k req/s.

szmarczak · 2020-03-02T17:04:29Z

package.json

@@ -10,7 +10,7 @@
 		"node": ">=10"
 	},
 	"scripts": {
-		"test": "xo && tsc --noEmit && nyc ava",
+		"test": "xo && tsc --noEmit && nyc --reporter=html --reporter=text ava",


Very useful to debug later. Generates both HTML output and text output as usual.

szmarczak · 2020-03-02T17:05:36Z

source/as-promise/core.ts

+import Request, {knownHookEvents, RequestError, HTTPError} from '../core';
+
+if (!knownHookEvents.includes('beforeRetry' as any)) {
+	knownHookEvents.push('beforeRetry' as any, 'afterResponse' as any);


The normalizing will be done via Request.normalizeArguments so we just tell Request that it needs to normalize beforeRetry too.

szmarczak · 2020-03-02T17:06:59Z

source/as-promise/core.ts

+		if (this._throwHttpErrors && !isHTTPError) {
+			this.destroy(error);
+		} else {
+			this.emit('error', error);
+		}


We cannot destroy this if the retry functionality is enabled. This is the responsibility of the Got promise.

szmarczak · 2020-03-02T17:08:28Z

source/as-promise/index.ts

+
+				// Download body
+				try {
+					body = await getStream.buffer(request);


Note that this is request, not response. If we were downloading directly from the response there would be no progress events emitted.

szmarczak · 2020-03-02T17:10:59Z

source/as-promise/types.ts

@@ -0,0 +1,149 @@
+import PCancelable = require('p-cancelable');


This file just adds promise-specific options.

szmarczak · 2020-03-02T17:12:10Z

source/as-stream.ts

@@ -1,144 +0,0 @@
-import duplexer3 = require('duplexer3');


Got is based directly on Duplex streams.

szmarczak · 2020-03-02T17:15:26Z

source/as-stream.ts

-		}
-
-		if (options.encoding) {
-			proxy.setEncoding(options.encoding);


Breaking change: options.encoding will no longer affect streams (it's the same behavior as in Got 9), people should do got.stream(...).setEncoding(encoding) instead. It should be mentioned in the docs that this option is for promises only.

Should also be documented as a doc comment on the options.encoding type.

szmarczak · 2020-03-02T17:16:02Z

source/as-stream.ts

-		// We cannot use `stream.pipeline(...)` here,
-		// because if we did then `output` would throw
-		// the original error before throwing `ReadError`.
-		response.pipe(output);


The new implementation doesn't do this. It just proxies the _read calls (basically a Duplex stream).

szmarczak · 2020-03-02T17:16:57Z

source/core/utils/options-to-url.ts

@@ -0,0 +1,73 @@
+/* istanbul ignore file: deprecated */


Note: searchParams is handled via the Request.normalizeArguments, so it won't be present here (as well as options.username and options.password).

szmarczak · 2020-03-02T17:22:04Z

source/errors.ts

-import {TimeoutError as TimedOutError} from './utils/timed-out';
-import {Response, NormalizedOptions} from './types';
-
-export class GotError extends Error {


Breaking: renamed GotError to RequestError

szmarczak · 2020-03-02T17:28:57Z

source/normalize-arguments.ts

-	options.decompress = Boolean(options.decompress);
-	options.isStream = Boolean(options.isStream);
-	options.throwHttpErrors = Boolean(options.throwHttpErrors);
-	options.ignoreInvalidCookies = Boolean(options.ignoreInvalidCookies);
-	options.cache = options.cache ?? false;
-	options.responseType = options.responseType ?? 'text';
-	options.resolveBodyOnly = Boolean(options.resolveBodyOnly);
-	options.followRedirect = Boolean(options.followRedirect);
-	options.dnsCache = options.dnsCache ?? false;
-	options.useElectronNet = Boolean(options.useElectronNet);
-	options.methodRewriting = Boolean(options.methodRewriting);
-	options.allowGetBody = Boolean(options.allowGetBody);


This has been dropped to get a 1.5k req/s increase. It will be inherited from defaults so it's unnecessary either because the defaults define these booleans.

szmarczak · 2020-03-02T17:34:40Z

source/progress.ts

-import {Transform as TransformStream} from 'stream';
-import is from '@sindresorhus/is';
-
-export function createProgressStream(name: 'downloadProgress' | 'uploadProgress', emitter: EventEmitter, totalBytes?: number | string): TransformStream {


This has been removed in favor of the Duplex stream (yeah, we can do things like this there).

szmarczak · 2020-03-02T17:35:26Z

source/request-as-event-emitter.ts

-	// `request.aborted` is a boolean since v11.0.0: https://github.com/nodejs/node/commit/4b00c4fafaa2ae8c41c1f78823c0feb810ae4723#diff-e3bc37430eb078ccbafe3aa3b570c91a
-	const isAborted = (): boolean => typeof currentRequest.aborted === 'number' || (currentRequest.aborted as unknown as boolean);
-
-	const emitError = async (error: GeneralError): Promise<void> => {


This has been renamed to request._beforeError.

szmarczak · 2020-03-02T17:36:29Z

source/request-as-event-emitter.ts

-
-				isPiped = true;
-
-				await pipeline(


We don't need to use pipeline if we do Duplex.

szmarczak · 2020-03-02T17:36:55Z

source/request-as-event-emitter.ts

-		}
-	};
-
-	emitter.retry = error => {


This has been moved to as-promise/index.ts.

szmarczak · 2020-03-02T17:53:30Z

source/types.ts

-
-	Note: Not available when the response is cached. This is hopefully a temporary limitation, see [lukechilds/cacheable-request#86](https://github.com/lukechilds/cacheable-request/issues/86).
-	*/
-	ip: string;


This has been corrected to ip?: string;

szmarczak · 2020-03-02T17:54:21Z

source/types.ts

-export interface RetryOptions extends Partial<DefaultRetryOptions> {
-	retries?: number;
-}


Why is it even here? It has been removed in Got 10...

szmarczak · 2020-03-02T17:56:09Z

source/types.ts

-export interface AgentByProtocol {
-	http?: http.Agent;
-	https?: https.Agent;
-}


Breaking change: options.agent can no longer be an instance of http.Agent. Instead, you need to pass an object like:

{ http: new http.Agent(...), https: new https.Agent(...), http2: new http2wrapper.Agent(...) }

Can you mention all the breaking changes in the PR description?

Will do! :D

source/types.ts

szmarczak · 2020-04-12T08:37:31Z

Can you double-check that you have ported all commits landed on master since you opened this PR into this branch?

Done. Nothing's missing as of now.

Also double-checked the types and the logic in general. Didn't notice any bugs other than those in the PR description (just nitpicks, nothing breaking).

sindresorhus · 2020-04-12T11:23:08Z

Wow! This includes a lot of fixes and improvements. Really nice work, @szmarczak! 🎉

joebowbeer · 2020-04-12T18:27:24Z

You might want to list the fix for #1090 as a breaking change in case someone was depending on forwarded authorization headers.

NMinhNguyen · 2020-06-16T13:23:47Z

package.json

@@ -7,10 +7,10 @@
 	"funding": "https://github.com/sindresorhus/got?sponsor=1",
 	"main": "dist/source",
 	"engines": {
-		"node": ">=10"
+		"node": ">=10.19.0"


@szmarczak Sorry to be commenting on a merged PR, but is there a reason you had to bump this? This should probably be mentioned as a breaking change?

This was referenced Feb 8, 2020

[WIP] HTTP2 support #832

Closed

A way to customize cacheable-lookup #1058

Closed

Help debugging ECONNRESET problems #1062

Closed

Cancel request before actual request #1012

Closed

sithmel mentioned this pull request Feb 18, 2020

Enhance the cache option #875

Open

6 tasks

szmarczak force-pushed the rework branch from 8302615 to 2567987 Compare February 28, 2020 19:09

szmarczak commented Mar 2, 2020

View reviewed changes

source/request-as-event-emitter.ts

isPiped = true;

await pipeline(

Copy link

Collaborator Author

szmarczak Mar 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need to use pipeline if we do Duplex.

szmarczak commented Mar 2, 2020

View reviewed changes

source/request-as-event-emitter.ts

}

};

emitter.retry = error => {

Copy link

Collaborator Author

szmarczak Mar 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has been moved to as-promise/index.ts.

szmarczak commented Mar 2, 2020

View reviewed changes

source/types.ts Show resolved Hide resolved

szmarczak commented Mar 2, 2020

View reviewed changes

source/types.ts Show resolved Hide resolved

szmarczak added 2 commits April 12, 2020 10:19

Make form-data a dev dependency

be086da

Fix FormData type

04821b9

szmarczak added 16 commits April 12, 2020 11:31

Update docs

ff197c1

Improve docs

827eaf0

Remove double new line break

5619b26

docs nitpick

ab273e6

docs nitpick v2

841ddda

docs nitpick v3

822bf88

document some HTTP options

3f7aacd

nitpick v4

3a15eb8

nitpick v5

a76bb61

nitpick v6

6912f0c

nitpick v7

b9f0592

nitpick v8

0d6b958

the query-string will be converted into URLSearchParams anyway

84467bf

fix v8

d84cc03

fix v9

90edfd1

fix dead urls

e4ac3a5

sindresorhus merged commit 286e31e into sindresorhus:master Apr 12, 2020

sindresorhus mentioned this pull request Apr 12, 2020

test: test case for issue #1120 #1121

Closed

4 tasks

This was referenced Apr 12, 2020

Server side aborted request with Transfer-Encoding: chunked never resolve/reject #1096

Closed

Error response body is not decompressed for streams #1142

Closed

mastermatt mentioned this pull request May 5, 2020

ClientRequest.destroy is not properly handled in Node 14 (breaks Got 11) nock/nock#1992

Closed

kschat mentioned this pull request May 19, 2020

Provide an overload for unknown responseType #1276

Merged

3 tasks

NMinhNguyen reviewed Jun 16, 2020

View reviewed changes

NMinhNguyen mentioned this pull request Jun 16, 2020

Node.js 10 support #1319

Closed

1 task

tehshane mentioned this pull request Feb 23, 2021

Parameter auth is deprecated. Use username / password instead. saintedlama/toggl-client#9

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite Got #1051

Rewrite Got #1051

szmarczak commented Feb 6, 2020 •

edited by issuehunt-oss bot

Loading

sindresorhus commented Feb 6, 2020

szmarczak commented Feb 6, 2020

szmarczak Mar 2, 2020 •

edited

Loading

szmarczak Mar 2, 2020

szmarczak Mar 2, 2020

szmarczak Mar 2, 2020

szmarczak Mar 2, 2020

szmarczak Mar 2, 2020

szmarczak Mar 2, 2020

szmarczak Mar 2, 2020 •

edited

Loading

szmarczak Mar 2, 2020

sindresorhus Mar 3, 2020

szmarczak Mar 2, 2020

szmarczak Mar 2, 2020 •

edited

Loading

szmarczak Mar 2, 2020

szmarczak Mar 2, 2020

szmarczak Mar 2, 2020

szmarczak Mar 2, 2020

szmarczak Mar 2, 2020

szmarczak Mar 2, 2020

szmarczak Mar 2, 2020

szmarczak Mar 2, 2020

szmarczak Mar 2, 2020 •

edited

Loading

sindresorhus Mar 3, 2020

szmarczak Mar 5, 2020

szmarczak commented Apr 12, 2020

sindresorhus commented Apr 12, 2020

joebowbeer commented Apr 12, 2020

NMinhNguyen Jun 16, 2020

		// got - promise x 3,092 ops/sec ±5.25% (73 runs sampled)
		// got - stream x 4,313 ops/sec ±5.61% (72 runs sampled)

		@@ -0,0 +1,149 @@
		import PCancelable = require('p-cancelable');

Rewrite Got #1051

Rewrite Got #1051

Conversation

szmarczak commented Feb 6, 2020 • edited by issuehunt-oss bot Loading

Changelog

Bug fixes

Breaking changes

API

Types

Enhancements

Miscellaneous

Known bugs

TODO

FEEL FREE TO TEST THIS

Referenced issues

sindresorhus commented Feb 6, 2020

szmarczak commented Feb 6, 2020

szmarczak Mar 2, 2020 • edited Loading

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

szmarczak Mar 2, 2020 • edited Loading

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

szmarczak Mar 2, 2020 • edited Loading

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

Choose a reason for hiding this comment

szmarczak Mar 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

szmarczak commented Apr 12, 2020

sindresorhus commented Apr 12, 2020

joebowbeer commented Apr 12, 2020

Choose a reason for hiding this comment

szmarczak commented Feb 6, 2020 •

edited by issuehunt-oss bot

Loading

szmarczak Mar 2, 2020 •

edited

Loading

szmarczak Mar 2, 2020 •

edited

Loading

szmarczak Mar 2, 2020 •

edited

Loading

szmarczak Mar 2, 2020 •

edited

Loading