Improve merging options #539

szmarczak · 2018-07-27T12:16:29Z

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.

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-30T14:55:36Z

readme.md

@@ -124,7 +124,7 @@ Default: `{}`

 Request headers.

-Existing headers will be overwritten. Headers set to `null` or `undefined` will be omitted.


I've removed that part ("or undefined") to not confuse people with the behaviour of got.mergeOptions, where setting to undefined may keep the old value.

sindresorhus · 2018-07-31T10:10:39Z

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 · 2018-07-31T10:16:16Z

Not sure I see the point of this. Example use-case? I generally prefer not exposing this unless absolutely necessary.

Simple example:

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.

sindresorhus · 2018-07-31T10:23:01Z

Your example doesn't really convince me otherwise. That problem can be solved without using this too. I think it should be up to users to bind methods if they want to use this.

szmarczak · 2018-07-31T10:27:11Z

I think it should be up to users to bind methods if they want to use this.

Right, I'll revert that :)

sindresorhus · 2018-08-02T07:31:02Z

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


Shouldn't this be 'user-agent': undefined?

Yeah, must missed that when was merging with master. Fixed.

sindresorhus · 2018-08-02T07:35:20Z

readme.md

@@ -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.**


Why was this removed? I guess we could shorten it a little bit, but I like to keep the note about you should treat objects passed to these methods as immutable. somewhere.

I consider it's no longer needed, because the defaults are cloned, so your object is safe, nothing will change :) This proves it.

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

szmarczak changed the title ~~Better got.assignOptions()~~ Rework got.assignOptions() and document it Jul 27, 2018

szmarczak requested a review from sindresorhus July 27, 2018 12:20

sindresorhus closed this Jul 27, 2018

szmarczak changed the title ~~Rework got.assignOptions() and document it~~ [WIP] Rework got.assignOptions() and document it Jul 30, 2018

szmarczak reopened this Jul 30, 2018

szmarczak changed the title ~~[WIP] Rework got.assignOptions() and document it~~ Rework got.assignOptions() and document it Jul 30, 2018

Improvements

3c294d0

szmarczak force-pushed the assignOptions-mergeWith branch from b9db06e to 3c294d0 Compare July 30, 2018 14:40

Fix a typo

31e97f9

szmarczak commented Jul 30, 2018

View reviewed changes

Repository owner deleted a comment from szmarczak Jul 30, 2018

szmarczak requested a review from brandon93s July 31, 2018 09:48

szmarczak force-pushed the assignOptions-mergeWith branch from 3c6ccad to 31e97f9 Compare July 31, 2018 10:31

szmarczak and others added 4 commits July 31, 2018 13:41

Revert calling hooks by reference

5332ae9

Merge branch 'master' into assignOptions-mergeWith

9c94752

Update create.js

d587954

Update readme.md

7a43333

szmarczak changed the title ~~Rework got.assignOptions() and document it~~ Improve merging options Jul 31, 2018

Remove unused merge-options.js

f1807b4

szmarczak removed the request for review from sindresorhus July 31, 2018 11:58

szmarczak requested review from sindresorhus and removed request for brandon93s July 31, 2018 11:58

sindresorhus added 2 commits August 2, 2018 14:29

Update headers.js

35bb0d6

Update headers.js

68ad4d2

sindresorhus reviewed Aug 2, 2018

View reviewed changes

Update readme.md

5bdf7a2

sindresorhus reviewed Aug 2, 2018

View reviewed changes

Update headers.js

6fbb9c4

sindresorhus merged commit af5c3fd into sindresorhus:master Aug 2, 2018

sindresorhus mentioned this pull request Aug 2, 2018

docs: use bold sparingly to ease visual parsing #540

Closed

szmarczak deleted the assignOptions-mergeWith branch January 17, 2019 18:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve merging options #539

Improve merging options #539

szmarczak commented Jul 27, 2018 •

edited

Loading

szmarczak Jul 30, 2018 •

edited

Loading

sindresorhus commented Jul 31, 2018

szmarczak commented Jul 31, 2018

sindresorhus commented Jul 31, 2018

szmarczak commented Jul 31, 2018

sindresorhus Aug 2, 2018 •

edited

Loading

szmarczak Aug 2, 2018

sindresorhus Aug 2, 2018

szmarczak Aug 2, 2018 •

edited

Loading

sindresorhus Aug 2, 2018

		@@ -124,7 +124,7 @@ Default: `{}`

		Request headers.

		Existing headers will be overwritten. Headers set to `null` or `undefined` will be omitted.

		@@ -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.

Improve merging options #539

Improve merging options #539

Conversation

szmarczak commented Jul 27, 2018 • edited Loading

szmarczak Jul 30, 2018 • edited Loading

Choose a reason for hiding this comment

sindresorhus commented Jul 31, 2018

szmarczak commented Jul 31, 2018

sindresorhus commented Jul 31, 2018

szmarczak commented Jul 31, 2018

sindresorhus Aug 2, 2018 • edited Loading

Choose a reason for hiding this comment

szmarczak Aug 2, 2018

Choose a reason for hiding this comment

sindresorhus Aug 2, 2018

Choose a reason for hiding this comment

szmarczak Aug 2, 2018 • edited Loading

Choose a reason for hiding this comment

sindresorhus Aug 2, 2018

Choose a reason for hiding this comment

szmarczak commented Jul 27, 2018 •

edited

Loading

szmarczak Jul 30, 2018 •

edited

Loading

sindresorhus Aug 2, 2018 •

edited

Loading

szmarczak Aug 2, 2018 •

edited

Loading