Repository: sindresorhus/got
Pull request #539 (2018-07-27 12:16:29)

Improve merging options
Compared to #538:

- clones `defaults` on instance creation,
- more clear docs a bit,
- `merge()` clones array properties too, which is useful when hooks need to access `hooks.beforeRequest.someProperty`,
- `merge()` is a different file now,
- `[]` instead of `new Array` in `merge()`,
- added `use strict` to `merge.js`,
- `merge()` is now an arrow function.

sindresorhus: Closing this in favor of #538

There's really no point in opening a new PR. We have not yet decided which direction to go and when we do, you should give @jstewmon a chance to implement it first.
szmarczak: Let's see where it heads then.
sindresorhus: Your implementation might be better but you have no description in this PR on how it's better. It's also forking our existing discussion into a new place, which makes it more difficult to keep track of.
sindresorhus: > Let's see where it heads then.

If we're not happy with the other implementation and it's going nowhere, we can just reopne this, but I'd like to give people a chance to finish their work.
szmarczak: I completely agree with you.

> I'd like to give people a chance to finish their work.

There's nothing wrong with helping, is it? The thing is I pushed a commit. He could look at what I did and improve it, but he **did not** want it, he reverted all the changes. He wanted to rediscover things which already have been discovered, so feel free. The same goal is achieved, but it's been delayed for days by this way.
sindresorhus: > There's nothing wrong with helping, is it? The thing is I pushed a commit. He could look at what I did and improve it, but he did not want it, he reverted all the changes. He wanted to rediscover things which already have been discovered, so feel free. The same goal is achieved, but it's been delayed for days by this way.

You pushed a commit with something we hadn't agreed on into his branch, which made it impossible for him to continue with his own solution. The correct way would have been to do your changes in a branch and link to your branch and explain in his PR how your way is different and better.
szmarczak: > The correct way would have been to do your changes in a branch and link to your branch and explain in his PR how your way is different and better.

I guess he would close it without reading any content, as he told me to do not so before. Then he said on gitter he will follow the way I had been talking before (fastify similar hooks) :) Seriously, no sense.
sindresorhus: > hooks are called by reference,

Not sure I see the point of this. Example use-case? I generally prefer not exposing `this` unless absolutely necessary.

> merge() is a different file now,

Can you leave it where it is for now so we get a diff?

> [] instead of new Array in merge().

👍 
szmarczak: > Not sure I see the point of this. Example use-case? I generally prefer not exposing this unless absolutely necessary.

Simple example:

```js
const {beforeRequest} = options.hooks;

const handler = function (options) {
	options.headers['some-data'] = this.data; // this = options.hooks.beforeRequest
};

beforeRequest.data = 'someData';

beforeRequest.push(handler);
```

> Can you leave it where it is for now so we get a diff?

Working on it.
szmarczak: > I think it should be up to users to bind methods if they want to use this.

Right, I'll revert that :)

Patch:
diff --git a/advanced-creation.md b/advanced-creation.md
index e781a91..0bef1fc 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.mergeOptions(defaults.options, options)`](readme.md#gotmergeOptionsparentoptions-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
diff --git a/readme.md b/readme.md
index 9982cda..b028cb9 100644
--- a/readme.md
+++ b/readme.md
@@ -124,7 +124,7 @@ Default: `{}`
 
 Request headers.
 
-Existing headers will be overwritten. Headers set to `null` or `undefined` will be omitted.
+Existing headers will be overwritten. Headers set to `null` will be omitted.
 
 ###### stream
 
@@ -358,7 +358,7 @@ Sets `options.method` to the method name and makes a request.
 
 #### got.extend([options])
 
-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).
+Configure a new `got` instance with default `options`. `options` are merged with the parent instance's `defaults.options` using [`got.mergeOptions`](#gotmergeoptionsparentoptions-newoptions).
 
 
 ```js
@@ -403,30 +403,28 @@ client.get('/demo');
 
 *Need more control over the behavior of Got? Check out the [`got.create()`](advanced-creation.md).*
 
-**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.**
-
 #### 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:
+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:
 
 ```js
-const a = {headers: {cat: 'meow', habitat: ['house', 'alley']}};
-const b = {headers: {cow: 'moo', habitat: ['barn']}};
+const a = {headers: {cat: 'meow', wolf: ['bark', 'wrrr']}};
+const b = {headers: {cow: 'moo', wolf: ['auuu']}};
 
-{...a, ...b}            // => {headers: {cow: 'moo'}}
-got.mergeOptions(a, b) // => {headers: {cat: 'meow', cow: 'moo', habitat: ['barn']}}
+{...a, ...b}            // => {headers: {cow: 'moo', wolf: ['auuu']}}
+got.mergeOptions(a, b)  // => {headers: {cat: 'meow', cow: 'moo', wolf: ['auuu']}}
 ```
 
+Options are deeply merged to a new object. The value of each key is determined as follows:
+
+- If the new property is set to `undefined`, it keeps the old one.
+- If the parent property is an instance of `URL` and the new value is a `string` or `URL`, a new URL instance is created: [`new URL(new, parent)`](https://developer.mozilla.org/en-US/docs/Web/API/URL/URL#Syntax).
+- If the new property is a plain `Object`:
+	- If the parent property is a plain `Object` too, both values are merged recursively into a new `Object`.
+	- Otherwise, only the new value is deeply cloned.
+- If the new property is an `Array`, it overwrites the old one with a deep clone of the new property.
+- Otherwise, the new value is assigned to the key.
+
 ## Errors
 
 Each error contains (if available) `statusCode`, `statusMessage`, `host`, `hostname`, `method`, `path`, `protocol` and `url` properties to make debugging easier.
diff --git a/source/create.js b/source/create.js
index 2cca4a7..1d86b8a 100644
--- a/source/create.js
+++ b/source/create.js
@@ -1,16 +1,18 @@
 'use strict';
 const errors = require('./errors');
-const mergeOptions = require('./merge-options');
 const asStream = require('./as-stream');
 const asPromise = require('./as-promise');
 const normalizeArguments = require('./normalize-arguments');
+const merge = require('./merge');
 const deepFreeze = require('./deep-freeze');
 
 const next = options => options.stream ? asStream(options) : asPromise(options);
+const mergeOptions = (defaults, options = {}) => merge({}, defaults, options);
 
 const create = defaults => {
+	defaults = merge({}, defaults);
 	if (!defaults.handler) {
-		defaults.handler = (options, next) => next(options);
+		defaults.handler = next;
 	}
 
 	function got(url, options) {
diff --git a/source/merge-options.js b/source/merge.js
similarity index 52%
rename from source/merge-options.js
rename to source/merge.js
index 91d231c..1402307 100644
--- a/source/merge-options.js
+++ b/source/merge.js
@@ -1,25 +1,16 @@
+'use strict';
 const {URL} = require('url');
 const is = require('@sindresorhus/is');
 
-module.exports = (defaults, options = {}) => {
-	return merge({}, defaults, options);
-};
-
-function merge(target, ...sources) {
+const 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];
+		for (const [key, sourceValue] of Object.entries(source)) {
 			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)
-			)) {
+
+			const targetValue = target[key];
+			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)) {
@@ -27,10 +18,15 @@ function merge(target, ...sources) {
 				} else {
 					target[key] = merge({}, sourceValue);
 				}
+			} else if (is.array(sourceValue)) {
+				target[key] = merge([], sourceValue);
 			} else {
 				target[key] = sourceValue;
 			}
 		}
 	}
+
 	return target;
-}
+};
+
+module.exports = merge;


Test patch:
diff --git a/test/create.js b/test/create.js
index 4004757..bcd60c5 100644
--- a/test/create.js
+++ b/test/create.js
@@ -77,26 +77,18 @@ test('custom headers (extend)', async t => {
 	t.is(headers.unicorn, 'rainbow');
 });
 
-test('extend overwrites arrays', t => {
+test('extend overwrites arrays with a deep clone', t => {
 	const statusCodes = [408];
 	const a = got.extend({retry: {statusCodes}});
-	t.deepEqual(a.defaults.options.retry.statusCodes, statusCodes);
+	statusCodes[0] = 500;
+	t.deepEqual(a.defaults.options.retry.statusCodes, [408]);
 	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}});
+test('extend keeps the old value if the new one is undefined', t => {
+	const a = got.extend({headers: undefined});
 	t.deepEqual(
-		b.defaults.options.headers,
+		a.defaults.options.headers,
 		got.defaults.options.headers
 	);
 });
@@ -107,12 +99,6 @@ test('extend merges URL instances', t => {
 	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: {},
@@ -161,3 +147,20 @@ test('no tampering with defaults', t => {
 	t.is(instance.defaults.options.baseUrl, 'example');
 	t.is(instance2.defaults.options.baseUrl, 'example');
 });
+
+test('defaults are cloned on instance creation', t => {
+	const options = {foo: 'bar'};
+	const methods = ['get'];
+	const instance = got.create({
+		methods,
+		options
+	});
+
+	t.notThrows(() => {
+		options.foo = 'foo';
+		methods[0] = 'post';
+	});
+
+	t.not(options.foo, instance.defaults.options.foo);
+	t.not(methods[0], instance.defaults.methods[0]);
+});
diff --git a/test/headers.js b/test/headers.js
index c95a726..8c8319a 100644
--- a/test/headers.js
+++ b/test/headers.js
@@ -142,12 +142,22 @@ test('remove null value headers', async t => {
 	t.false(Reflect.has(headers, 'user-agent'));
 });
 
-test('remove undefined value headers', async t => {
+test('setting a header to undefined keeps the old value', async t => {
 	const {body} = await got(s.url, {
 		headers: {
-			foo: undefined
+			'user-agent': undefined
 		}
 	});
 	const headers = JSON.parse(body);
-	t.false(Reflect.has(headers, 'foo'));
+	t.not(headers['user-agent'], undefined);
+});
+
+test('non-existent headers set to undefined are omitted', async t => {
+	const {body} = await got(s.url, {
+		headers: {
+			blah: undefined
+		}
+	});
+	const headers = JSON.parse(body);
+	t.false(Reflect.has(headers, 'blah'));
 });