Make `got.mergeOptions()` behavior more obvious and document its behavior #538

jstewmon · 2018-07-25T22:57:38Z

Option merging is now consistent, so null header removal is done during
options normalization. null could not be used to indicate property
removal because null is a significant value for some settings.

jstewmon · 2018-07-25T23:01:37Z

source/assign-options.js

-		}
+function customizer(objValue, srcValue) {
+	if (is.array(srcValue) || is.array(objValue)) {
+		return cloneDeep(srcValue);


This might not be necessary. Maybe we can just pick the right target based on whether is.array(objValue). I was running short on time, so I did this to get it working.

I think we could ditch cloneDeep as a dep if that works.

szmarczak · 2018-07-26T13:33:30Z

source/assign-options.js

-		for (const [key, value] of Object.entries(options.headers)) {
-			if (is.nullOrUndefined(value)) {
-				delete returnOptions.headers[key];
+function merge(target = {}, ...sources) {


target = {} is useless here

szmarczak · 2018-07-26T13:34:07Z

test/create.js

+	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');
+});


szmarczak · 2018-07-26T13:39:30Z

source/assign-options.js

@@ -1,27 +1,30 @@
-const extend = require('extend');
+const url = require('url');


use const {URL} = require('url');

szmarczak · 2018-07-26T13:51:30Z

test/create.js

+	const a = got.extend({retry: {statusCodes}});
+	t.deepEqual(a.defaults.options.retry.statusCodes, statusCodes);
+	t.is(a.defaults.options.retry.statusCodes, statusCodes);
+});


Consider this example:

test('extend overwrites arrays', t => { let statusCodes = [408]; const a = got.extend({retry: {statusCodes}}); statusCodes[0] = 500; t.deepEqual(a.defaults.options.retry.statusCodes, [408]); t.not(a.defaults.options.retry.statusCodes, statusCodes); });

It shouldn't fail. It should deep copy arrays IMO.

Oh. Defaults are deep frozen here:

got/source/create.js

Line 45 in da7f055

value: deepFreeze(defaults),

and ain't cloned before.

Yeah, I made it only deep copy plain object, since all other types are merged by replacement.

Yeah, I made it only deep copy plain object, since all other types are merged by replacement.

I don't think obvious things need an explanation, do they? You've just repeated yourself: that's what your code does, doesn't it? Please come up with a real argument. I'll illustrate if you have problems with understanding mine:

Someone creates an array.

The array is passed to assignOptions through got.extend().

It is passed by reference (please keep that word in mind my friend).

It's deep frozen (got.extend() -> got.create() -> deepFreeze())

Someone wants to reuse the array.

💥

I understand that concern, but I intentionally kept the implementation simple, so that the results are predictable.

I was also concerned with performance, since this method is called not only by got.extend and got.create, but also by every call to got().

There are an infinite number of possible reference types that inherit from object, which may also have unexpected behavior when cloned, especially when only an object's own properties are cloned.

I reluctantly chose to recursively merge source values that are plain objects when the target value is not a plain object for this reason because normalize-arguments currently requires that the options it receives are mutable. I would rather have refactored normalize-arguments, so that it selectively builds a new object from the input, but I thought that was too large a change to introduce here.

Do you think it would be helpful to explicitly mention in the got.extend and got.create that the resulting object is frozen, so that any reference types will be frozen? We could also mention that a workaround is to use something like _.deepClone if users need to preserve a mutable instance of a config setting.

We could also reconsider whether freezing the options tips the balance of predicable behavior nearer or farther from the user's expectations.

Just clarify my position - I'm not strictly inflexible on the implementation, but I want to make sure my rationale is clearly expressed before we decide to change it.

@sindresorhus do you have an opinion on this topic?

I was also concerned with performance, since this method is called not only by got.extend and got.create, but also by every call to got().

Then why? There are many nitpicks that could improve performance a bit (it's unnoticeable) like this one and you're nothing about that. Please, be serious.

Do you think it would be helpful to explicitly mention in the got.extend and got.create that the resulting object is frozen, so that any reference types will be frozen?

No. That's dumb. All plain objects are frozen and other objects like arrays are not? That doesn't have any sense. BTW, It's OK to pass references to other types than objects and arrays since got doesn't use them.

Do you think it would be helpful to explicitly mention in the got.extend and got.create that the resulting object is frozen, so that any reference types will be frozen? We could also mention that a workaround is to use something like _.deepClone if users need to preserve a mutable instance of a config setting.

Yes, that would be useful.

szmarczak · 2018-07-26T14:03:09Z

test/create.js

+	t.true('headers' in got.defaults.options);
+	const a = got.extend({headers: undefined});
+	t.false('headers' in a.defaults.options);
+});


👎 It should keep the old value.

I'm fine either way on this one.

I interpreted the previous behavior as undefined being a sentinel value that can be used to remove a setting, i.e. a discrete feature of got's extension behavior.

There is a test that verifies that null headers are omitted from the normalized arguments, but I've addressed that case in normalize-arguments.

@sindresorhus do you have a preference?

Look at the other merge / extend libs :) They omit undefined values. I mean users would think that if you leave another property undefined it will remain unchanged. It's header-specific.

Just to clarify, based on your complete suggestion below, you think undefined values in the source should be noop and the target value should be preserved, right?

That is what lodash.merge does, but I don't understand the rationale in that approach. If the key exists in the source, I think it expresses some intent of the caller vs the value being undefined b/c the key does not exist.

I would suggest that when the key exists in the source and the value is undefined, the result should either be:

omit the key from the result

set the key to undefined in the result

I could be persuaded that the source value should be ignored, but I the rationale isn't obvious to me.

Just to clarify, based on your complete suggestion below, you think undefined values in the source should be noop and the target value should be preserved, right?

yes.

What's the rationale for, or problem solved, by deep merging arrays?

You don't even simply clone them, so let's start with that. With arrays there should be no exception. I'm not gonna explain simple things.

I really appreciate you want to do it on your own :)

I'm not gonna explain simple things.

Arguments are worthless if the other part doesn't understand you. Not everyone is in your exact mindset. We all need to be on the same page. I was also confused by your comment.

I just realized that my proposal is how lodash.merge works.

But, I also can't think of a reason why an object would contain a key with the value set to undefined in this context b/c the key could simply be omitted to get the same behavior.

It allows you to do inline conditionals:

foo({ unicorn: checkForUnicorns() && '🦄' });

Without, I would have to do:

const options = {}; if (shouldUseUnicorns()) { options.unicorn = '🦄'; } foo(options);

I'm not yet sure it's worth the divergence from Object.assign/object-spread. There's value having the same semantics as the native methods. Especially when our method is using the word assign.

I backed off your change b/c it overreached what we had discussed. I don't mind maintainer commits for nits or agreed upon changes, but please don't make behavioral changes when consensus has not been reached.

@szmarczak Don't modify other people's PRs before we have agreed on a solution.

@sindresorhus thanks for providing that example. I can appreciate the convenience that provides.

It occurred to me that, in the future, we could expose a symbol support deleting a key if the need for it arises. Similarly, we could expose a symbol to indicate that the result should be undefined if there's a need to explicitly set a key to undefined.

Using symbols for those cases would have the disadvantage of being a unique approach, but it would have the advantage of our interface being optimized for the most common scenarios.

If we're settled on ignoring undefined, we could rename assignOptions to mergeOptions to help clarify that its behavior is not congruent with Object.assign.

Arguments are worthless if the other part doesn't understand you. Not everyone is in your exact mindset. We all need to be on the same page. I was also confused by your comment.

I'll sum up: going through shortcuts won't end well, just a hint. Another hint: references, arrays, deep freeze. This says a lot.

Don't modify other people's PRs before we have agreed on a solution.

I've just proposed a solution based on yours. Do what you want, I'm off. I'll be happy with any (current one is cool too).

szmarczak · 2018-07-26T14:07:30Z

test/create.js

+		b.defaults.options.headers,
+		{...got.defaults.options.headers, ...{baz: 'baz', bar: null}}
+	);
+});


👎 The feature is meant only for headers. Otherwise, if the new value is undefined, it shouldn't be replaced.

Ack. This is the same behavior as we're discussing above, so we'll resolve this when we resolve that.

szmarczak

I suggest this:

const merge = (target, ...sources) => {
	for (const source of sources) {
		for (const [key, sourceValue] of Object.entries(source)) {
			const targetValue = target[key];
			if (!is.undefined(sourceValue)) {
				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 if (is.array(sourceValue)) {
					target[key] = merge([], sourceValue);
				} else {
					target[key] = sourceValue;
				}
			}
		}
	}

	return target;
};

szmarczak · 2018-07-26T14:35:28Z

readme.md

@@ -408,14 +406,23 @@ client.get('/demo');

 #### got.assignOptions(parentOptions, newOptions)

-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:


I'd leave this note about avoiding object spread because mostly that's not what users want.

I pruned it because I didn't think it was necessary for us to document the language feature, and the reader's attention to detail is inversely related to the length of the docs.

I can put it back if you feel strongly that it will meaningfully assist users in making the right choice.

@sindresorhus What are your thoughts on that?

I think it's worth keeping. I see the mistake of using Object.assign/object-spread for deep objects all the time. This also gives context to why we have this custom method.

On closer inspection, I didn't actually remove this - I just inserted the function description before the warning. :-)

Option merging is now consistent, so null header removal is done during options normalization. null could not be used to indicate property removal because null is a significant value for some settings.

We have a narrow set of rules for merging objects and lodash's more aggressive merge may have unintended consequences when the values being merged are not plain objects.

szmarczak · 2018-07-27T11:20:33Z

test/headers.js

-	});
-	const headers = JSON.parse(body);
-	t.false(Reflect.has(headers, 'user-agent'));
-});


I wouldn't support this, as it might be confusing with got.extend({ ... }), where setting to undefined keeps the old value.

This test had to be rewritten to use a header key that doesn't have a default value. The behavior is consistent as you wished. :-)

szmarczak · 2018-07-28T10:36:47Z

test/headers.js

@@ -145,9 +145,9 @@ test('remove null value headers', async t => {
 test('remove undefined value headers', async t => {


Wait. What's with user-agent? You need to change the test name if you change the behaviour too.

szmarczak · 2018-07-28T10:38:24Z

test/create.js

+});
+
+test('extend ignores object values set to undefined (root keys)', t => {
+	t.true('headers' in got.defaults.options);


Use Reflect.has() for consistency.

Also I suggest figuring out a better test name.

szmarczak · 2018-07-28T10:40:12Z

test/create.js

+	t.is(a.defaults.options.retry.statusCodes, statusCodes);
+});
+
+test('extend overwrites null', t => {


After I read the test name I still don't know what's this about.

szmarczak · 2018-07-28T10:42:56Z

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.assignOptions`](#got.assignoptionsparentoptions,-newoptions).


Stop guessing. You need to change the URL to #gotassignoptionsparentoptions-newoptions.

sindresorhus · 2018-07-29T07:59:41Z

I've decided. Let's go with ignoring undefined and rename the method to mergeOptions.

I think we should also deep clone the array, not because it solves all the problems, but it makes it less likely that the user will encounter a problem with references, and I think that is worth the neglible performance we save by not doing it. This is also how lodash.merge works and what users would expect:

Array and plain object properties are merged recursively. Other objects and value types are overridden by assignment. Source objects are applied from left to right. - lodahs.merge docs

We can pretty much reuse the above description for our mergeOptions docs.

jstewmon · 2018-07-29T19:33:26Z

@sindresorhus I wasn't sure whether you meant to just make a copy of the source array or merge the array into the target's property value (like lodash). I assumed (and implemented) the former, since that's closest to what we've been discussing, but your comment about lodash gave me some doubt.

szmarczak · 2018-07-29T20:11:23Z

@jstewmon You forgot git add ;) There's no merge-options file.

- rename assignOptions to mergeOptions - update docs accordingly

szmarczak · 2018-07-30T08:28:31Z

@jstewmon I would send a review, but you've blocked me. 🎉 Congratz.

Avoid new Array. See this for more explanation.
What does extend overwrites null mean?

Also please test markdown links because the ones you provided don't work.

It's no longer applicable.

sindresorhus · 2018-07-31T09:54:35Z

I wasn't sure whether you meant to just make a copy of the source array or merge the array into the target's property value

I meant, make a copy, yes.

sindresorhus · 2018-07-31T10:06:36Z

Thanks for the PR, @jstewmon :)

jstewmon commented Jul 25, 2018

View reviewed changes

jstewmon changed the title ~~use lodash to merge options and document assignOptions behaviors~~ make assignOptions behavior more obvious and document its behavior Jul 26, 2018

jstewmon force-pushed the assignOptions-mergeWith branch from d9e04cd to 6f893d6 Compare July 26, 2018 04:25

szmarczak reviewed Jul 26, 2018

View reviewed changes

szmarczak requested changes Jul 26, 2018

View reviewed changes

szmarczak reviewed Jul 26, 2018

View reviewed changes

Jonathan Stewmon and others added 3 commits July 26, 2018 11:06

use lodash to merge options and document assignOptions behaviors

26fdae4

Option merging is now consistent, so null header removal is done during options normalization. null could not be used to indicate property removal because null is a significant value for some settings.

refactor: use our own merge instead of lodash

3bf4693

We have a narrow set of rules for merging objects and lodash's more aggressive merge may have unintended consequences when the values being merged are not plain objects.

refactor: dereference URL on import, remove pointless target default

a40b948

jstewmon force-pushed the assignOptions-mergeWith branch from 1695ada to a40b948 Compare July 26, 2018 16:06

sindresorhus changed the title ~~make assignOptions behavior more obvious and document its behavior~~ Make got.assignOptions() behavior more obvious and document its behavior Jul 27, 2018

szmarczak requested review from sindresorhus and brandon93s July 27, 2018 11:13

szmarczak approved these changes Jul 27, 2018

View reviewed changes

szmarczak reviewed Jul 27, 2018

View reviewed changes

szmarczak force-pushed the assignOptions-mergeWith branch from 95c510f to 8665f51 Compare July 27, 2018 11:34

jstewmon force-pushed the assignOptions-mergeWith branch from 8665f51 to a40b948 Compare July 27, 2018 12:07

szmarczak mentioned this pull request Jul 27, 2018

Improve merging options #539

Merged

Jonathan Stewmon and others added 2 commits July 27, 2018 14:17

feat: ignore undefined values in assignOptions

ecbb640

chore: add test for extends null value

8acd5a7

szmarczak reviewed Jul 28, 2018

View reviewed changes

- recursively merge arrays to a new array

27788f9

- rename assignOptions to mergeOptions - update docs accordingly

jstewmon force-pushed the assignOptions-mergeWith branch from ea30ff2 to 27788f9 Compare July 29, 2018 20:17

Update create.js

1452651

sindresorhus changed the title ~~Make got.assignOptions() behavior more obvious and document its behavior~~ Make got.mergeOptions() behavior more obvious and document its behavior Jul 31, 2018

sindresorhus merged commit 5562be2 into sindresorhus:master Jul 31, 2018

Repository owner locked as too heated and limited conversation to collaborators Jul 31, 2018

jstewmon deleted the assignOptions-mergeWith branch August 24, 2018 17:11

		@@ -1,27 +1,30 @@
		const extend = require('extend');
		const url = require('url');

		@@ -408,14 +406,23 @@ client.get('/demo');

		#### got.assignOptions(parentOptions, newOptions)

		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:

		@@ -145,9 +145,9 @@ test('remove null value headers', async t => {
		test('remove undefined value headers', async t => {

Make got.mergeOptions() behavior more obvious and document its behavior #538

Make got.mergeOptions() behavior more obvious and document its behavior #538

Conversation

jstewmon commented Jul 25, 2018

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

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

Choose a reason for hiding this comment

sindresorhus Jul 27, 2018 • 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

szmarczak left a 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

sindresorhus Jul 27, 2018 • 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

sindresorhus commented Jul 29, 2018

jstewmon commented Jul 29, 2018

szmarczak commented Jul 29, 2018 • edited Loading

szmarczak commented Jul 30, 2018 • edited Loading

sindresorhus commented Jul 31, 2018 • edited Loading

sindresorhus commented Jul 31, 2018

Make `got.mergeOptions()` behavior more obvious and document its behavior #538

Make `got.mergeOptions()` behavior more obvious and document its behavior #538

sindresorhus Jul 27, 2018 •

edited

Loading

sindresorhus Jul 27, 2018 •

edited

Loading

szmarczak commented Jul 29, 2018 •

edited

Loading

szmarczak commented Jul 30, 2018 •

edited

Loading

sindresorhus commented Jul 31, 2018 •

edited

Loading