From d369b083f7e84dd5c771fd95ff9f1fbab1d4dfca Mon Sep 17 00:00:00 2001
From: Jonathan Stewmon <jstewmon@gmail.com>
Date: Tue, 31 Jul 2018 05:06:12 -0500
Subject: [PATCH] Make `got.mergeOptions()` behavior more obvious and document
 its behavior (#538)

---
 advanced-creation.md          |  6 +++---
 package.json                  |  1 -
 readme.md                     | 28 +++++++++++++++++--------
 source/assign-options.js      | 27 ------------------------
 source/create.js              | 10 ++++-----
 source/merge-options.js       | 36 ++++++++++++++++++++++++++++++++
 source/normalize-arguments.js |  8 ++++++-
 test/create.js                | 39 ++++++++++++++++++++++++++++++++++-
 test/headers.js               |  4 ++--
 9 files changed, 110 insertions(+), 49 deletions(-)
 delete mode 100644 source/assign-options.js
 create mode 100644 source/merge-options.js

diff --git a/advanced-creation.md b/advanced-creation.md
index b7a8993ea..e781a91e9 100644
--- a/advanced-creation.md
+++ b/advanced-creation.md
@@ -11,7 +11,7 @@ Configure a new `got` instance with the provided settings.<br>
 
 ##### [options](readme.md#options)
 
-To inherit from parent, set it as `got.defaults.options` or use [`got.assignOptions(defaults.options, options)`](readme.md#gotassignoptionsparentoptions-newoptions).<br>
+To inherit from parent, set it as `got.defaults.options` or use [`got.mergeOptions(defaults.options, options)`](readme.md#gotmergeOptionsparentoptions-newoptions).<br>
 **Note**: Avoid using [object spread](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Spread_syntax#Spread_in_object_literals) as it doesn't work recursively.
 
 ##### methods
@@ -54,7 +54,7 @@ const settings = {
 		return next(options);
 	},
 	methods: got.defaults.methods,
-	options: got.assignOptions(got.defaults.options, {
+	options: got.mergeOptions(got.defaults.options, {
 		json: true
 	})
 };
@@ -99,7 +99,7 @@ const unchangedGot = got.create(defaults);
 const settings = {
 	handler: got.defaults.handler,
 	methods: got.defaults.methods,
-	options: got.assignOptions(got.defaults.options, {
+	options: got.mergeOptions(got.defaults.options, {
 		headers: {
 			unicorn: 'rainbow'
 		}
diff --git a/package.json b/package.json
index 14e56b9ab..2062d2bc8 100644
--- a/package.json
+++ b/package.json
@@ -38,7 +38,6 @@
 		"cacheable-request": "^4.0.1",
 		"decompress-response": "^3.3.0",
 		"duplexer3": "^0.1.4",
-		"extend": "^3.0.1",
 		"get-stream": "^3.0.0",
 		"mimic-response": "^1.0.0",
 		"p-cancelable": "^0.5.0",
diff --git a/readme.md b/readme.md
index d1795528e..9982cda27 100644
--- a/readme.md
+++ b/readme.md
@@ -358,10 +358,8 @@ Sets `options.method` to the method name and makes a request.
 
 #### got.extend([options])
 
-Configure a new `got` instance with default `options` and (optionally) a custom `baseUrl`:
+Configure a new `got` instance with default `options`. `options` are merged with the extended instance's `defaults.options` as described in [`got.mergeOptions`](#gotmergeoptionsparentoptions-newoptions).
 
-**Note:** You can extend another extended instance. `got.defaults` provides settings used by that instance.<br>
-Check out the [unchanged default values](source/index.js).
 
 ```js
 const client = got.extend({
@@ -405,16 +403,28 @@ client.get('/demo');
 
 *Need more control over the behavior of Got? Check out the [`got.create()`](advanced-creation.md).*
 
-#### got.assignOptions(parentOptions, newOptions)
+**Both `got.extend(options)` and `got.create(options)` will freeze the instance's default options. For `got.extend()`, the instance's default options are the result of `got.mergeOptions`, which effectively copies plain `Object` and `Array` values. Therefore, you should treat objects passed to these methods as immutable.**
 
-Extends parent options. Avoid using [object spread](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Spread_syntax#Spread_in_object_literals) as it doesn't work recursively:
+#### got.mergeOptions(parentOptions, newOptions)
+
+Extends parent options. The options objects are deeply merged to a new object. The value of each property is determined as follows:
+
+- If the new value is `undefined` the parent value is preserved.
+- If the parent value is an instance of `URL` and the new value is a `string` or `URL`, a new URL instance is created, using the parent value as the base: `new URL(new, parent)`.
+- If the new value is an `Array`, the new value is recursively merged into an empty array (the source value is discarded). `undefined` elements in the source array are not assigned during the merge, so the resulting array will have an empty item where the source array had an `undefined` item.
+- If the new value is a plain `Object`
+  - If the parent value is a plain `Object`, both values are merged recursively into a new `Object`.
+  - Otherwise, only the new value is merged recursively into a new `Object`.
+- Otherwise, the new value is assigned to the property.
+
+Avoid using [object spread](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Spread_syntax#Spread_in_object_literals) as it doesn't work recursively:
 
 ```js
-const a = {headers: {cat: 'meow'}};
-const b = {headers: {dog: 'woof'}};
+const a = {headers: {cat: 'meow', habitat: ['house', 'alley']}};
+const b = {headers: {cow: 'moo', habitat: ['barn']}};
 
-{...a, ...b}            // => {headers: {dog: 'woof'}}
-got.assignOptions(a, b) // => {headers: {cat: 'meow', dog: 'woof'}}
+{...a, ...b}            // => {headers: {cow: 'moo'}}
+got.mergeOptions(a, b) // => {headers: {cat: 'meow', cow: 'moo', habitat: ['barn']}}
 ```
 
 ## Errors
diff --git a/source/assign-options.js b/source/assign-options.js
deleted file mode 100644
index 176876ce2..000000000
--- a/source/assign-options.js
+++ /dev/null
@@ -1,27 +0,0 @@
-const extend = require('extend');
-const is = require('@sindresorhus/is');
-
-module.exports = (defaults, options = {}) => {
-	const returnOptions = extend(true, {}, defaults, options);
-
-	if (Reflect.has(options, 'headers')) {
-		for (const [key, value] of Object.entries(options.headers)) {
-			if (is.nullOrUndefined(value)) {
-				delete returnOptions.headers[key];
-			}
-		}
-	}
-
-	// Override these arrays because we don't want to extend them
-	if (is.object(options.retry)) {
-		if (Reflect.has(options.retry, 'methods')) {
-			returnOptions.retry.methods = options.retry.methods;
-		}
-
-		if (Reflect.has(options.retry, 'statusCodes')) {
-			returnOptions.retry.statusCodes = options.retry.statusCodes;
-		}
-	}
-
-	return returnOptions;
-};
diff --git a/source/create.js b/source/create.js
index 2d09394de..2cca4a7dd 100644
--- a/source/create.js
+++ b/source/create.js
@@ -1,6 +1,6 @@
 'use strict';
 const errors = require('./errors');
-const assignOptions = require('./assign-options');
+const mergeOptions = require('./merge-options');
 const asStream = require('./as-stream');
 const asPromise = require('./as-promise');
 const normalizeArguments = require('./normalize-arguments');
@@ -15,7 +15,7 @@ const create = defaults => {
 
 	function got(url, options) {
 		try {
-			options = assignOptions(defaults.options, options);
+			options = mergeOptions(defaults.options, options);
 			return defaults.handler(normalizeArguments(url, options, defaults), next);
 		} catch (error) {
 			return Promise.reject(error);
@@ -24,13 +24,13 @@ const create = defaults => {
 
 	got.create = create;
 	got.extend = (options = {}) => create({
-		options: assignOptions(defaults.options, options),
+		options: mergeOptions(defaults.options, options),
 		methods: defaults.methods,
 		handler: defaults.handler
 	});
 
 	got.stream = (url, options) => {
-		options = assignOptions(defaults.options, options);
+		options = mergeOptions(defaults.options, options);
 		options.stream = true;
 		return defaults.handler(normalizeArguments(url, options, defaults), next);
 	};
@@ -40,7 +40,7 @@ const create = defaults => {
 		got.stream[method] = (url, options) => got.stream(url, {...options, method});
 	}
 
-	Object.assign(got, {...errors, assignOptions});
+	Object.assign(got, {...errors, mergeOptions});
 	Object.defineProperty(got, 'defaults', {
 		value: deepFreeze(defaults),
 		writable: false,
diff --git a/source/merge-options.js b/source/merge-options.js
new file mode 100644
index 000000000..91d231c68
--- /dev/null
+++ b/source/merge-options.js
@@ -0,0 +1,36 @@
+const {URL} = require('url');
+const is = require('@sindresorhus/is');
+
+module.exports = (defaults, options = {}) => {
+	return merge({}, defaults, options);
+};
+
+function merge(target, ...sources) {
+	for (const source of sources) {
+		const sourceIter = is.array(source) ?
+			source.entries() :
+			Object.entries(source);
+		for (const [key, sourceValue] of sourceIter) {
+			const targetValue = target[key];
+			if (is.undefined(sourceValue)) {
+				continue;
+			}
+			if (is.array(sourceValue)) {
+				target[key] = merge(new Array(sourceValue.length), sourceValue);
+			} else if (is.urlInstance(targetValue) && (
+				is.urlInstance(sourceValue) || is.string(sourceValue)
+			)) {
+				target[key] = new URL(sourceValue, targetValue);
+			} else if (is.plainObject(sourceValue)) {
+				if (is.plainObject(targetValue)) {
+					target[key] = merge({}, targetValue, sourceValue);
+				} else {
+					target[key] = merge({}, sourceValue);
+				}
+			} else {
+				target[key] = sourceValue;
+			}
+		}
+	}
+	return target;
+}
diff --git a/source/normalize-arguments.js b/source/normalize-arguments.js
index b890f0a97..d22436649 100644
--- a/source/normalize-arguments.js
+++ b/source/normalize-arguments.js
@@ -67,11 +67,17 @@ module.exports = (url, options, defaults) => {
 		options.headers.accept = 'application/json';
 	}
 
+	const {headers} = options;
+	for (const [key, value] of Object.entries(headers)) {
+		if (is.nullOrUndefined(value)) {
+			delete headers[key];
+		}
+	}
+
 	const {body} = options;
 	if (is.nullOrUndefined(body)) {
 		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');
diff --git a/test/create.js b/test/create.js
index 17ccc2b3f..4004757b3 100644
--- a/test/create.js
+++ b/test/create.js
@@ -1,3 +1,4 @@
+import {URL} from 'url';
 import test from 'ava';
 import got from '../source';
 import {createServer} from './helpers/server';
@@ -76,6 +77,42 @@ test('custom headers (extend)', async t => {
 	t.is(headers.unicorn, 'rainbow');
 });
 
+test('extend overwrites arrays', t => {
+	const statusCodes = [408];
+	const a = got.extend({retry: {statusCodes}});
+	t.deepEqual(a.defaults.options.retry.statusCodes, statusCodes);
+	t.not(a.defaults.options.retry.statusCodes, statusCodes);
+});
+
+test('extend overwrites null', t => {
+	const statusCodes = null;
+	const a = got.extend({retry: {statusCodes}});
+	t.is(a.defaults.options.retry.statusCodes, statusCodes);
+});
+
+test('extend ignores source values set to undefined', t => {
+	const a = got.extend({
+		headers: {foo: undefined, 'user-agent': undefined}
+	});
+	const b = a.extend({headers: {foo: undefined}});
+	t.deepEqual(
+		b.defaults.options.headers,
+		got.defaults.options.headers
+	);
+});
+
+test('extend merges URL instances', t => {
+	const a = got.extend({baseUrl: new URL('https://example.com')});
+	const b = a.extend({baseUrl: '/foo'});
+	t.is(b.defaults.options.baseUrl.toString(), 'https://example.com/foo');
+});
+
+test('extend ignores object values set to undefined (root keys)', t => {
+	t.true(Reflect.has(got.defaults.options, 'headers'));
+	const a = got.extend({headers: undefined});
+	t.deepEqual(a.defaults.options, got.defaults.options);
+});
+
 test('create', async t => {
 	const instance = got.create({
 		options: {},
@@ -105,7 +142,7 @@ test('no tampering with defaults', t => {
 	const instance = got.create({
 		handler: got.defaults.handler,
 		methods: got.defaults.methods,
-		options: got.assignOptions(got.defaults.options, {
+		options: got.mergeOptions(got.defaults.options, {
 			baseUrl: 'example'
 		})
 	});
diff --git a/test/headers.js b/test/headers.js
index 80b39e22d..c95a726d5 100644
--- a/test/headers.js
+++ b/test/headers.js
@@ -145,9 +145,9 @@ test('remove null value headers', async t => {
 test('remove undefined value headers', async t => {
 	const {body} = await got(s.url, {
 		headers: {
-			'user-agent': undefined
+			foo: undefined
 		}
 	});
 	const headers = JSON.parse(body);
-	t.false(Reflect.has(headers, 'user-agent'));
+	t.false(Reflect.has(headers, 'foo'));
 });