Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 7 commits into from
Jul 31, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions advanced-creation.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
})
};
Expand Down Expand Up @@ -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'
}
Expand Down
1 change: 0 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
28 changes: 19 additions & 9 deletions readme.md
Original file line number Diff line number Diff line change
Expand Up @@ -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`](#got.mergeOptionsparentoptions,-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({
Expand Down Expand Up @@ -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
Expand Down
27 changes: 0 additions & 27 deletions source/assign-options.js

This file was deleted.

10 changes: 5 additions & 5 deletions source/create.js
Original file line number Diff line number Diff line change
@@ -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');
Expand All @@ -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);
Expand All @@ -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);
};
Expand All @@ -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,
Expand Down
36 changes: 36 additions & 0 deletions source/merge-options.js
Original file line number Diff line number Diff line change
@@ -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;
}
8 changes: 7 additions & 1 deletion source/normalize-arguments.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down
39 changes: 38 additions & 1 deletion test/create.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import {URL} from 'url';
import test from 'ava';
import got from '../source';
import {createServer} from './helpers/server';
Expand Down Expand Up @@ -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');
});
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍


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: {},
Expand Down Expand Up @@ -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'
})
});
Expand Down
4 changes: 2 additions & 2 deletions test/headers.js
Original file line number Diff line number Diff line change
Expand Up @@ -145,9 +145,9 @@ test('remove null value headers', async t => {
test('remove undefined value headers', async t => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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