Repository: sindresorhus/got
Pull request #647 (2018-10-28 15:33:39)

Change the way to retry using the afterResponse hook


Patch:
diff --git a/readme.md b/readme.md
index 0655cb5..8742d4e 100644
--- a/readme.md
+++ b/readme.md
@@ -203,9 +203,36 @@ Parse response body with `JSON.parse` and set `accept` header to `application/js
 
 ###### query
 
-Type: `string` `Object` [`URLSearchParams`](https://developer.mozilla.org/en-US/docs/Web/API/URLSearchParams)
+Type: `string` `Object<string, string|number>` [`URLSearchParams`](https://developer.mozilla.org/en-US/docs/Web/API/URLSearchParams)
 
-Query string object that will be added to the request URL. This will override the query string in `url`.
+Query string that will be added to the request URL. This will override the query string in `url`.
+
+If you need to pass in an array, you can do it using a `URLSearchParams` instance:
+
+```js
+const got = require('got');
+
+const query = new URLSearchParams([['key', 'a'], ['key', 'b']]);
+
+got('https://example.com', {query});
+
+console.log(query.toString());
+//=> 'key=a&key=b'
+```
+
+And if you need a different array format, you could use the [`query-string`](https://github.com/sindresorhus/query-string) package:
+
+```js
+const got = require('got');
+const queryString = require('query-string');
+
+const query = queryString.stringify({key: ['a', 'b']}, {arrayFormat: 'bracket'});
+
+got('https://example.com', {query});
+
+console.log(query);
+//=> 'key[]=a&key[]=b'
+```
 
 ###### timeout
 
@@ -383,7 +410,9 @@ got('example.com', {
 Type: `Function[]`<br>
 Default: `[]`
 
-Called with [response object](#response). Each function should return the response or updated options. This is especially useful when you want to refresh an access token. Example:
+Called with [response object](#response) and a retry function.
+
+Each function should return the response. This is especially useful when you want to refresh an access token. Example:
 
 ```js
 const got = require('got');
@@ -391,7 +420,7 @@ const got = require('got');
 const instance = got.extend({
 	hooks: {
 		afterResponse: [
-			response => {
+			(response, retryWithMergedOptions) => {
 				if (response.statusCode === 401) { // Unauthorized
 					const updatedOptions = {
 						headers: {
@@ -402,8 +431,8 @@ const instance = got.extend({
 					// Save for further requests
 					instance.defaults.options = got.mergeOptions(instance.defaults.options, updatedOptions);
 
-					// Apply changes
-					return updatedOptions;
+					// Make a new retry
+					return retryWithMergedOptions(updatedOptions);
 				}
 
 				// No changes otherwise
diff --git a/source/as-promise.js b/source/as-promise.js
index 931f284..cd11c5a 100644
--- a/source/as-promise.js
+++ b/source/as-promise.js
@@ -5,8 +5,10 @@ const is = require('@sindresorhus/is');
 const PCancelable = require('p-cancelable');
 const requestAsEventEmitter = require('./request-as-event-emitter');
 const {HTTPError, ParseError, ReadError} = require('./errors');
+const {options: mergeOptions} = require('./merge');
+const {reNormalize} = require('./normalize-arguments');
 
-module.exports = options => {
+const asPromise = options => {
 	const proxy = new EventEmitter();
 
 	const promise = new PCancelable((resolve, reject, onCancel) => {
@@ -27,28 +29,34 @@ module.exports = options => {
 				return;
 			}
 
-			const {statusCode} = response;
 			const limitStatusCode = options.followRedirect ? 299 : 399;
 
 			response.body = data;
 
 			try {
-				for (const hook of options.hooks.afterResponse) {
+				for (const [index, hook] of Object.entries(options.hooks.afterResponse)) {
 					// eslint-disable-next-line no-await-in-loop
-					response = await hook(response);
-
-					if (is.plainObject(response)) {
-						if (emitter.retry(response) === false) {
-							reject(new Error('Retry limit reached.'));
-						}
-						return;
-					}
+					response = await hook(response, updatedOptions => {
+						updatedOptions = reNormalize(mergeOptions(options, {
+							...updatedOptions,
+							retry: 0,
+							throwHttpErrors: false
+						}));
+
+						// Remove any further hooks for that request, because we we'll call them anyway.
+						// The loop continues. We don't want duplicates (asPromise recursion).
+						updatedOptions.hooks.afterResponse = options.hooks.afterResponse.slice(0, index);
+
+						return asPromise(updatedOptions);
+					});
 				}
 			} catch (error) {
 				reject(error);
 				return;
 			}
 
+			const {statusCode} = response;
+
 			if (options.json && response.body) {
 				try {
 					response.body = JSON.parse(response.body);
@@ -95,3 +103,5 @@ module.exports = options => {
 
 	return promise;
 };
+
+module.exports = asPromise;
diff --git a/source/normalize-arguments.js b/source/normalize-arguments.js
index 158ff53..ae79cc4 100644
--- a/source/normalize-arguments.js
+++ b/source/normalize-arguments.js
@@ -1,5 +1,6 @@
 'use strict';
 const {URL, URLSearchParams} = require('url'); // TODO: Use the `URL` global when targeting Node.js 10
+const urlLib = require('url');
 const is = require('@sindresorhus/is');
 const urlParseLax = require('url-parse-lax');
 const lowercaseKeys = require('lowercase-keys');
@@ -52,14 +53,14 @@ const preNormalize = (options, defaults) => {
 	delete options.timeout;
 
 	const {retry} = options;
-	if (defaults && retry !== false) {
+	options.retry = {
+		retries: 0,
+		methods: [],
+		statusCodes: []
+	};
+
+	if (!is.empty(defaults) && retry !== false) {
 		options.retry = {...defaults.retry};
-	} else {
-		options.retry = {
-			retries: 0,
-			methods: [],
-			statusCodes: []
-		};
 	}
 
 	if (retry !== false) {
@@ -85,14 +86,18 @@ const preNormalize = (options, defaults) => {
 	return options;
 };
 
-module.exports = (url, options, defaults) => {
+const normalize = (url, options, defaults) => {
 	if (is.plainObject(url) && Reflect.has(url, 'url')) {
 		options = url;
 		url = url.url;
 		delete options.url;
 	}
 
-	options = merge({}, defaults.options, options ? preNormalize(options, defaults.options) : {});
+	if (defaults) {
+		options = merge({}, defaults.options, options ? preNormalize(options, defaults.options) : {});
+	} else {
+		options = merge({}, options ? preNormalize(options) : {});
+	}
 
 	if (!is.string(url) && !is.object(url)) {
 		throw new TypeError(`Parameter \`url\` must be a string or object, not ${is(url)}`);
@@ -242,4 +247,8 @@ module.exports = (url, options, defaults) => {
 	return options;
 };
 
+const reNormalize = options => normalize(urlLib.format(options), options);
+
+module.exports = normalize;
 module.exports.preNormalize = preNormalize;
+module.exports.reNormalize = reNormalize;
diff --git a/source/request-as-event-emitter.js b/source/request-as-event-emitter.js
index 7071f9a..4e9bf94 100644
--- a/source/request-as-event-emitter.js
+++ b/source/request-as-event-emitter.js
@@ -226,16 +226,6 @@ module.exports = (options, input) => {
 	emitter.retry = error => {
 		let backoff;
 
-		if (is.plainObject(error)) {
-			// Retry with updated options
-			options = {
-				...options,
-				...error
-			};
-
-			error = null;
-		}
-
 		try {
 			backoff = options.retry.retries(++retryTries, error);
 		} catch (error2) {


Test patch:
diff --git a/test/hooks.js b/test/hooks.js
index 36a3a54..6b3324a 100644
--- a/test/hooks.js
+++ b/test/hooks.js
@@ -7,6 +7,8 @@ const errorString = 'oops';
 const error = new Error(errorString);
 let s;
 
+let visited401then500;
+
 test.before('setup', async () => {
 	s = await createServer();
 	const echoHeaders = (request, response) => {
@@ -40,6 +42,17 @@ test.before('setup', async () => {
 		response.end();
 	});
 
+	s.on('/401then500', (request, response) => {
+		if (visited401then500) {
+			response.statusCode = 500;
+		} else {
+			visited401then500 = true;
+			response.statusCode = 401;
+		}
+
+		response.end();
+	});
+
 	await s.listen(s.port);
 });
 
@@ -234,16 +247,15 @@ test('afterResponse allows modifications', async t => {
 
 test('afterResponse allows to retry', async t => {
 	const response = await got(`${s.url}/401`, {
-		json: true,
 		hooks: {
 			afterResponse: [
-				response => {
+				(response, retryWithMergedOptions) => {
 					if (response.statusCode === 401) {
-						return {
+						return retryWithMergedOptions({
 							headers: {
 								token: 'unicorn'
 							}
-						};
+						});
 					}
 
 					return response;
@@ -254,19 +266,67 @@ test('afterResponse allows to retry', async t => {
 	t.is(response.statusCode, 200);
 });
 
-test('throws on afterResponse when reached retry limit', async t => {
+test('no infinity loop when retrying on afterResponse', async t => {
 	await t.throwsAsync(got(`${s.url}/401`, {
 		retry: 0,
 		hooks: {
 			afterResponse: [
-				() => {
-					return {
+				(response, retryWithMergedOptions) => {
+					return retryWithMergedOptions({
 						headers: {
-							token: 'unicorn'
+							token: 'invalid'
 						}
-					};
+					});
+				}
+			]
+		}
+	}), {instanceOf: got.HTTPError, message: 'Response code 401 (Unauthorized)'});
+});
+
+test.serial('throws on afterResponse retry failure', async t => {
+	visited401then500 = false;
+
+	await t.throwsAsync(got(`${s.url}/401then500`, {
+		retry: 1,
+		hooks: {
+			afterResponse: [
+				(response, retryWithMergedOptions) => {
+					if (response.statusCode === 401) {
+						return retryWithMergedOptions({
+							headers: {
+								token: 'unicorn'
+							}
+						});
+					}
+
+					return response;
+				}
+			]
+		}
+	}), {instanceOf: got.HTTPError, message: 'Response code 500 (Internal Server Error)'});
+});
+
+test.serial('doesn\'t throw on afterResponse retry HTTP failure if throwHttpErrors is false', async t => {
+	visited401then500 = false;
+
+	const response = await got(`${s.url}/401then500`, {
+		throwHttpErrors: false,
+		retry: 1,
+		hooks: {
+			afterResponse: [
+				(response, retryWithMergedOptions) => {
+					if (response.statusCode === 401) {
+						return retryWithMergedOptions({
+							headers: {
+								token: 'unicorn'
+							}
+						});
+					}
+
+					return response;
 				}
 			]
 		}
-	}), 'Retry limit reached.');
+	});
+	t.is(response.statusCode, 500);
 });