Repository: sindresorhus/got (ts)

Issue:
Regression in 7.0.0 options processing
#217 loosened unnecessary restrictions on `body` option that were discussed in #212 and allowed it to be an object that is generally considered *plain* by a developer, i.e. not a *complex* object - a buffer or a stream, which are processed differently. 

#297 introduced a regression that imposed a restriction on `body` option again.

Excessive sanitization results from the use of `is-plain-obj` and restricts `body` object prototype chain to contain only  `Object.prototype` or `null`. This restriction results in counter-intuitive behaviour and requires a developer to use tricks like `Object.assign({}, obj)` instead of not doing them, even if an object is perceived as 'plain' (in my experience it was `querystring.parse()`) .

More importantly, this results in breaking changes that aren't reflected in change logs. The restrictions were dropped for a good reason, and resulting behaviour has been already relied on by package users. Now the regression just happened without an obvious reason.

Pull request description:
Loosen unnecessary restrictions on the `body` option
Fixes #381 

Patch:
diff --git a/readme.md b/readme.md
index ddc07c3..5e20aaf 100644
--- a/readme.md
+++ b/readme.md
@@ -258,6 +258,24 @@ Determines if a `got.HTTPError` is thrown for error responses (non-2xx status co
 
 If this is disabled, requests that encounter an error status code will be resolved with the `response` instead of throwing. This may be useful if you are checking for resource availability and are expecting error responses.
 
+###### hooks
+
+Type: `Object<string, Array<Function>>`<br>
+Default: `{ beforeRequest: [] }`
+
+Hooks allow modifications during the request lifecycle. Hook functions may be async and are run serially.
+
+###### hooks.beforeRequest
+
+Type: `Array<Function>`<br>
+Default: `[]`
+
+Called with the normalized request options. Got will make no further changes to the request before it is sent. This is especially useful in conjunction with [`got.extend()`](#instances) and [`got.create()`](advanced-creation.md) when you want to create an API client that uses HMAC-signing.
+
+See the [AWS section](#aws) for an example.
+
+**Note**: Modifying the `body` is not recommended because the `content-length` header has already been computed and assigned.
+
 #### Streams
 
 **Note**: Progress events, redirect events and request/response events can also be used with promises.
@@ -626,47 +644,35 @@ got('http://unix:/var/run/docker.sock:/containers/json');
 got('unix:/var/run/docker.sock:/containers/json');
 ```
 
+
 ## AWS
 
-Requests to AWS services need to have their headers signed. This can be accomplished by using the [`aws4`](https://www.npmjs.com/package/aws4) package. This is an example for querying an ["Elasticsearch Service"](https://aws.amazon.com/elasticsearch-service/) host with a signed request.
+Requests to AWS services need to have their headers signed. This can be accomplished by using the [`aws4`](https://www.npmjs.com/package/aws4) package. This is an example for querying an ["API Gateway"](https://docs.aws.amazon.com/apigateway/api-reference/signing-requests/) with a signed request.
 
 ```js
-const url = require('url');
 const AWS = require('aws-sdk');
 const aws4 = require('aws4');
 const got = require('got');
-const config = require('./config');
-
-// Reads keys from the environment or `~/.aws/credentials`. Could be a plain object.
-const awsConfig = new AWS.Config({ region: config.region });
 
-function request(url, options) {
-	const awsOpts = {
-		region: awsConfig.region,
-		headers: {
-			accept: 'application/json',
-			'content-type': 'application/json'
-		},
-		method: 'GET',
-		json: true
-	};
-
-	// We need to parse the URL before passing it to `got` so `aws4` can sign the request
-	options = {
-		...url.parse(url),
-		...awsOpts,
-		...options
-	};
-
-	aws4.sign(options, awsConfig.credentials);
-
-	return got(options);
-}
-
-request(`https://${config.host}/production/users/1`);
+const credentials = await new AWS.CredentialProviderChain().resolvePromise();
+
+// Create a Got instance to use relative paths and signed requests
+const awsClient = got.extend(
+	{
+		baseUrl: 'https://<api-id>.execute-api.<api-region>.amazonaws.com/<stage>/',
+		hooks: {
+			beforeRequest: [
+				async options => {
+					await credentials.getPromise();
+					aws4.sign(options, credentials);
+				}
+			]
+		}
+	}
+);
 
-request(`https://${config.host}/production/`, {
-	// All usual `got` options
+const response = await awsClient('endpoint/path', {
+	// Request-specific options
 });
 ```
 
diff --git a/source/index.js b/source/index.js
index 1f9f4c4..f4573f8 100644
--- a/source/index.js
+++ b/source/index.js
@@ -23,6 +23,9 @@ const defaults = {
 		throwHttpErrors: true,
 		headers: {
 			'user-agent': `${pkg.name}/${pkg.version} (https://github.com/sindresorhus/got)`
+		},
+		hooks: {
+			beforeRequest: []
 		}
 	}
 };
diff --git a/source/normalize-arguments.js b/source/normalize-arguments.js
index 82a329b..e09e97d 100644
--- a/source/normalize-arguments.js
+++ b/source/normalize-arguments.js
@@ -8,6 +8,7 @@ const urlToOptions = require('./url-to-options');
 const isFormData = require('./is-form-data');
 
 const retryAfterStatusCodes = new Set([413, 429, 503]);
+const knownHookEvents = ['beforeRequest'];
 
 module.exports = (url, options, defaults) => {
 	if (Reflect.has(options, 'url') || (is.object(url) && Reflect.has(url, 'url'))) {
@@ -63,25 +64,26 @@ module.exports = (url, options, defaults) => {
 		options.method = (options.method || 'GET').toUpperCase();
 	} else {
 		const {headers} = options;
+		const isObject = is.object(body) && !Buffer.isBuffer(body) && !is.nodeStream(body);
 		if (!is.nodeStream(body) && !is.string(body) && !is.buffer(body) && !(options.form || options.json)) {
 			throw new TypeError('The `body` option must be a stream.Readable, string or Buffer');
 		}
 
-		if (options.json && !(is.plainObject(body) || is.array(body))) {
-			throw new TypeError('The `body` option must be a plain Object or Array when the `json` option is used');
+		if (options.json && !(isObject || is.array(body))) {
+			throw new TypeError('The `body` option must be an Object or Array when the `json` option is used');
 		}
 
-		if (options.form && !is.plainObject(body)) {
-			throw new TypeError('The `body` option must be a plain Object when the `form` option is used');
+		if (options.form && !isObject) {
+			throw new TypeError('The `body` option must be an Object when the `form` option is used');
 		}
 
 		if (isFormData(body)) {
 			// Special case for https://github.com/form-data/form-data
 			headers['content-type'] = headers['content-type'] || `multipart/form-data; boundary=${body.getBoundary()}`;
-		} else if (options.form && is.plainObject(body)) {
+		} else if (options.form) {
 			headers['content-type'] = headers['content-type'] || 'application/x-www-form-urlencoded';
 			options.body = (new URLSearchParamsGlobal(body)).toString();
-		} else if (options.json && (is.plainObject(body) || is.array(body))) {
+		} else if (options.json) {
 			headers['content-type'] = headers['content-type'] || 'application/json';
 			options.body = JSON.stringify(body);
 		}
@@ -185,5 +187,31 @@ module.exports = (url, options, defaults) => {
 		delete options.timeout;
 	}
 
+	if (is.nullOrUndefined(options.hooks)) {
+		options.hooks = {};
+	}
+	if (is.object(options.hooks)) {
+		for (const hookEvent of knownHookEvents) {
+			const hooks = options.hooks[hookEvent];
+			if (is.nullOrUndefined(hooks)) {
+				options.hooks[hookEvent] = [];
+			} else if (is.array(hooks)) {
+				hooks.forEach(
+					(hook, index) => {
+						if (!is.function_(hook)) {
+							throw new TypeError(
+								`Parameter \`hooks.${hookEvent}[${index}]\` must be a function, not ${is(hook)}`
+							);
+						}
+					}
+				);
+			} else {
+				throw new TypeError(`Parameter \`hooks.${hookEvent}\` must be an array, not ${is(hooks)}`);
+			}
+		}
+	} else {
+		throw new TypeError(`Parameter \`hooks\` must be an object, not ${is(options.hooks)}`);
+	}
+
 	return options;
 };
diff --git a/source/request-as-event-emitter.js b/source/request-as-event-emitter.js
index 38c062c..80a9ee6 100644
--- a/source/request-as-event-emitter.js
+++ b/source/request-as-event-emitter.js
@@ -241,6 +241,11 @@ module.exports = (options = {}) => {
 				options.headers['content-length'] = uploadBodySize;
 			}
 
+			for (const hook of options.hooks.beforeRequest) {
+				// eslint-disable-next-line no-await-in-loop
+				await hook(options);
+			}
+
 			get(options);
 		} catch (error) {
 			emitter.emit('error', error);


Test patch:
diff --git a/test/arguments.js b/test/arguments.js
index 964941a..ab70ab2 100644
--- a/test/arguments.js
+++ b/test/arguments.js
@@ -101,6 +101,40 @@ test('throws TypeError when `url` is passed as an option', async t => {
 	await t.throws(got({url: 'example.com'}), {instanceOf: TypeError});
 });
 
+test('throws TypeError when `hooks` is not an object', async t => {
+	await t.throws(
+		() => got(s.url, {hooks: 'not object'}),
+		{
+			instanceOf: TypeError,
+			message: 'Parameter `hooks` must be an object, not string'
+		}
+	);
+});
+
+test('throws TypeError when known `hooks` value is not an array', async t => {
+	await t.throws(
+		() => got(s.url, {hooks: {beforeRequest: {}}}),
+		{
+			instanceOf: TypeError,
+			message: 'Parameter `hooks.beforeRequest` must be an array, not Object'
+		}
+	);
+});
+
+test('throws TypeError when known `hooks` array item is not a function', async t => {
+	await t.throws(
+		() => got(s.url, {hooks: {beforeRequest: [{}]}}),
+		{
+			instanceOf: TypeError,
+			message: 'Parameter `hooks.beforeRequest[0]` must be a function, not Object'
+		}
+	);
+});
+
+test('allows extra keys in `hooks`', async t => {
+	await t.notThrows(() => got(`${s.url}/test`, {hooks: {extra: {}}}));
+});
+
 test.after('cleanup', async () => {
 	await s.close();
 });
diff --git a/test/error.js b/test/error.js
index fdebb9d..0ba0bfa 100644
--- a/test/error.js
+++ b/test/error.js
@@ -1,6 +1,7 @@
 import http from 'http';
 import test from 'ava';
 import sinon from 'sinon';
+import getStream from 'get-stream';
 import proxyquire from 'proxyquire';
 import got from '../source';
 import {createServer} from './helpers/server';
@@ -26,6 +27,11 @@ test.before('setup', async () => {
 		res.end('body');
 	});
 
+	s.on('/body', async (req, res) => {
+		const body = await getStream(req);
+		res.end(body);
+	});
+
 	await s.listen(s.port);
 });
 
@@ -53,18 +59,31 @@ test('dns message', async t => {
 });
 
 test('options.body error message', async t => {
-	const err = await t.throws(got(s.url, {body: {}}));
-	t.regex(err.message, /The `body` option must be a stream\.Readable, string or Buffer/);
+	await t.throws(got(s.url, {body: {}}), {
+		message: 'The `body` option must be a stream.Readable, string or Buffer'
+	});
 });
 
 test('options.body json error message', async t => {
-	const err = await t.throws(got(s.url, {body: Buffer.from('test'), json: true}));
-	t.regex(err.message, /The `body` option must be a plain Object or Array when the `json` option is used/);
+	await t.throws(got(s.url, {body: Buffer.from('test'), json: true}), {
+		message: 'The `body` option must be an Object or Array when the `json` option is used'
+	});
 });
 
 test('options.body form error message', async t => {
-	const err = await t.throws(got(s.url, {body: Buffer.from('test'), form: true}));
-	t.regex(err.message, /The `body` option must be a plain Object when the `form` option is used/);
+	await t.throws(got(s.url, {body: Buffer.from('test'), form: true}), {
+		message: 'The `body` option must be an Object when the `form` option is used'
+	});
+});
+
+test('no plain object restriction on body', async t => {
+	function CustomObject() {
+		this.a = 123;
+	}
+
+	const {body} = await got(`${s.url}/body`, {body: new CustomObject(), json: true});
+
+	t.deepEqual(body, {a: 123});
 });
 
 test('default status message', async t => {
@@ -83,9 +102,7 @@ test.serial('http.request error', async t => {
 	const stub = sinon.stub(http, 'request').callsFake(() => {
 		throw new TypeError('The header content contains invalid characters');
 	});
-	const err = await t.throws(got(s.url));
-	t.true(err instanceof got.RequestError);
-	t.is(err.message, 'The header content contains invalid characters');
+	await t.throws(got(s.url), {instanceOf: got.RequestError, message: 'The header content contains invalid characters'});
 	stub.restore();
 });
 
@@ -99,8 +116,7 @@ test.serial('catch error in mimicResponse', async t => {
 		'mimic-response': mimicResponse
 	});
 
-	const err = await t.throws(proxiedGot(s.url));
-	t.is(err.message, 'Error in mimic-response');
+	await t.throws(proxiedGot(s.url), {message: 'Error in mimic-response'});
 });
 
 test.after('cleanup', async () => {
diff --git a/test/hooks.js b/test/hooks.js
new file mode 100644
index 0000000..220bf97
--- /dev/null
+++ b/test/hooks.js
@@ -0,0 +1,105 @@
+import test from 'ava';
+import delay from 'delay';
+import {createServer} from './helpers/server';
+import got from '..';
+
+let s;
+
+test.before('setup', async () => {
+	s = await createServer();
+	const echoHeaders = (req, res) => {
+		res.statusCode = 200;
+		res.write(JSON.stringify(req.headers));
+		res.end();
+	};
+	s.on('/', echoHeaders);
+	await s.listen(s.port);
+});
+
+test('beforeRequest receives normalized options', async t => {
+	await got(
+		s.url,
+		{
+			json: true,
+			hooks: {
+				beforeRequest: [
+					options => {
+						t.is(options.path, '/');
+						t.is(options.hostname, 'localhost');
+					}
+				]
+			}
+		}
+	);
+});
+
+test('beforeRequest allows modifications', async t => {
+	const res = await got(
+		s.url,
+		{
+			json: true,
+			hooks: {
+				beforeRequest: [
+					options => {
+						options.headers.foo = 'bar';
+					}
+				]
+			}
+		}
+	);
+	t.is(res.body.foo, 'bar');
+});
+
+test('beforeRequest awaits async function', async t => {
+	const res = await got(
+		s.url,
+		{
+			json: true,
+			hooks: {
+				beforeRequest: [
+					async options => {
+						await delay(100);
+						options.headers.foo = 'bar';
+					}
+				]
+			}
+		}
+	);
+	t.is(res.body.foo, 'bar');
+});
+
+test('beforeRequest rejects when beforeRequest throws', async t => {
+	await t.throws(
+		() => got(s.url, {
+			hooks: {
+				beforeRequest: [
+					() => {
+						throw new Error('oops');
+					}
+				]
+			}
+		}),
+		{
+			instanceOf: Error,
+			message: 'oops'
+		}
+	);
+});
+
+test('beforeRequest rejects when beforeRequest rejects', async t => {
+	await t.throws(
+		() => got(s.url, {
+			hooks: {
+				beforeRequest: [() => Promise.reject(new Error('oops'))]
+			}
+		}),
+		{
+			instanceOf: Error,
+			message: 'oops'
+		}
+	);
+});
+
+test.after('cleanup', async () => {
+	await s.close();
+});