Fix got.mergeOptions() regression

Fixes #1211

Author: szmarczak

readme.md

@@ -1132,17 +1132,12 @@ const handler = (options, next) => {
 const instance = got.extend({handlers: [handler]});
 ```
 
-#### got.extend(...instances)
+#### got.extend(...options, ...instances, ...)
 
 Merges many instances into a single one:
-- options are merged using [`got.mergeOptions()`](#gotmergeoptionsparentoptions-newoptions) (+ hooks are merged too),
+- options are merged using [`got.mergeOptions()`](#gotmergeoptionsparentoptions-newoptions) (including hooks),
 - handlers are stored in an array (you can access them through `instance.defaults.handlers`).
 
-#### got.extend(...options, ...instances, ...)
-
-It's possible to combine options and instances.\
-It gives the same effect as `got.extend(...options).extend(...instances)`:
-
 ```js
 const a = {headers: {cat: 'meow'}};
 const b = got.extend({
@@ -1159,7 +1154,7 @@ got.extend(a, b);
 //=> {headers: {cat: 'meow', cow: 'moo'}}
 ```
 
-#### got.mergeOptions(parentOptions, newOptions)
+#### got.mergeOptions(parent, ...sources)
 
 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:
 
@@ -1171,24 +1166,31 @@ const b = {headers: {cow: 'moo', wolf: ['auuu']}};
 got.mergeOptions(a, b)  // => {headers: {cat: 'meow', cow: 'moo', wolf: ['auuu']}}
 ```
 
+**Note:** Only Got options are merged! Custom user options should be defined via [`options.context`](#context).
+
 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 both properties are an instances of `URLSearchParams`, a new URLSearchParams instance is created. The values are merged using [`urlSearchParams.append(key, value)`](https://developer.mozilla.org/en-US/docs/Web/API/URLSearchParams/append).
-- 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 not defined, the old value is used.
+- If the new property is explicitly set to `undefined`:
+	- If the parent property is a plain `object`, the parent value is deeply cloned.
+	- Otherwise, `undefined` is used.
+- If the parent value is an instance of `URLSearchParams`:
+	- If the new value is a `string`, an `object` or an instance of `URLSearchParams`, a new `URLSearchParams` instance is created. The values are merged using [`urlSearchParams.append(key, value)`](https://developer.mozilla.org/en-US/docs/Web/API/URLSearchParams/append).
+	- Otherwise, the only available value is `undefined`.
 - 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.
 - Properties that are not enumerable, such as `context`, `body`, `json`, and `form`, will not be merged.
+- Otherwise, the new value is assigned to the key.
+
 ```js
 const a = {json: {cat: 'meow'}};
 const b = {json: {cow: 'moo'}};
 
 got.mergeOptions(a, b);
 //=> {json: {cow: 'moo'}}
 ```
-- Otherwise, the new value is assigned to the key.
 
 #### got.defaults
 
@@ -1203,7 +1205,7 @@ The Got defaults used in that instance.
 Type: `Function[]`\
 Default: `[]`
 
-An array of functions. You execute them directly by calling `got()`. They are some sort of "global hooks" - these functions are called first. The last handler (*it's hidden*) is either [`asPromise`](source/as-promise.ts) or [`asStream`](source/as-stream.ts), depending on the `options.isStream` property.
+An array of functions. You execute them directly by calling `got()`. They are some sort of "global hooks" - these functions are called first. The last handler (*it's hidden*) is either [`asPromise`](source/core/as-promise/index.ts) or [`asStream`](source/core/index.ts), depending on the `options.isStream` property.
 
 Each handler takes two arguments:
 

source/core/index.ts

@@ -572,8 +572,6 @@ export default class Request extends Duplex implements RequestEvents<Request> {
 
 	static normalizeArguments(url?: string | URL, options?: Options, defaults?: Defaults): NormalizedOptions {
 		const rawOptions = options;
-		const searchParameters = options?.searchParams;
-		const hooks = options?.hooks;
 
 		if (is.object(url) && !is.urlInstance(url)) {
 			options = {...defaults, ...(url as Options), ...options};
@@ -589,30 +587,6 @@ export default class Request extends Duplex implements RequestEvents<Request> {
 			}
 		}
 
-		// Prevent duplicating default search params & hooks
-		if (searchParameters === undefined) {
-			delete options.searchParams;
-		} else {
-			options.searchParams = searchParameters;
-		}
-
-		if (hooks === undefined) {
-			delete options.hooks;
-		} else {
-			options.hooks = hooks;
-		}
-
-		// Setting options to `undefined` turns off its functionalities
-		if (rawOptions && defaults) {
-			for (const key in rawOptions) {
-				// @ts-ignore Dear TypeScript, all object keys are strings (or symbols which are NOT enumerable).
-				if (is.undefined(rawOptions[key]) && !is.undefined(defaults[key])) {
-					// @ts-ignore See the note above
-					options[key] = defaults[key];
-				}
-			}
-		}
-
 		// TODO: Deprecate URL options in Got 12.
 
 		// Support extend-specific options
@@ -647,9 +621,7 @@ export default class Request extends Duplex implements RequestEvents<Request> {
 		}
 
 		// `options.headers`
-		if (is.undefined(options.headers)) {
-			options.headers = {};
-		} else if (options.headers === defaults?.headers) {
+		if (options.headers === defaults?.headers) {
 			options.headers = {...options.headers};
 		} else {
 			options.headers = lowercaseKeys({...(defaults?.headers), ...options.headers});
@@ -666,19 +638,19 @@ export default class Request extends Duplex implements RequestEvents<Request> {
 		}
 
 		// `options.searchParams`
-		if (options.searchParams) {
-			if (!is.string(options.searchParams) && !(options.searchParams instanceof URLSearchParams)) {
-				validateSearchParameters(options.searchParams);
-			}
+		if ('searchParams' in options) {
+			if (options.searchParams && options.searchParams !== defaults?.searchParams) {
+				if (!is.string(options.searchParams) && !(options.searchParams instanceof URLSearchParams)) {
+					validateSearchParameters(options.searchParams);
+				}
 
-			options.searchParams = new URLSearchParams(options.searchParams as Record<string, string>);
+				options.searchParams = new URLSearchParams(options.searchParams as Record<string, string>);
 
-			// `normalizeArguments()` is also used to merge options
-			defaults?.searchParams?.forEach((value, key) => {
-				(options!.searchParams as URLSearchParams).append(key, value);
-			});
-		} else {
-			options.searchParams = defaults?.searchParams;
+				// `normalizeArguments()` is also used to merge options
+				defaults?.searchParams?.forEach((value, key) => {
+					(options!.searchParams as URLSearchParams).append(key, value);
+				});
+			}
 		}
 
 		// `options.username` & `options.password`
@@ -811,6 +783,7 @@ export default class Request extends Duplex implements RequestEvents<Request> {
 		}
 
 		// `options.hooks`
+		const areHooksDefault = options.hooks === defaults?.hooks;
 		options.hooks = {...options.hooks};
 
 		for (const event of knownHookEvents) {
@@ -826,7 +799,7 @@ export default class Request extends Duplex implements RequestEvents<Request> {
 			}
 		}
 
-		if (defaults) {
+		if (defaults && !areHooksDefault) {
 			for (const event of knownHookEvents) {
 				const defaultHooks = defaults.hooks[event];
 

test/normalize-arguments.ts

@@ -1,3 +1,4 @@
+import {URL, URLSearchParams} from 'url';
 import test from 'ava';
 import got from '../source';
 
@@ -20,3 +21,26 @@ test('should copy non-numerable properties', t => {
 
 	t.is(mergedTwice.json, options.json);
 });
+
+test('should replace URLs', t => {
+	const options = got.mergeOptions({
+		url: new URL('http://localhost:41285'),
+		searchParams: new URLSearchParams('page=0')
+	}, {
+		url: 'http://localhost:41285/?page=1',
+		searchParams: undefined
+	});
+
+	const otherOptions = got.mergeOptions({
+		url: new URL('http://localhost:41285'),
+		searchParams: {
+			page: 0
+		}
+	}, {
+		url: 'http://localhost:41285/?page=1',
+		searchParams: undefined
+	});
+
+	t.is(options.url.href, 'http://localhost:41285/?page=1');
+	t.is(otherOptions.url.href, 'http://localhost:41285/?page=1');
+});

test/pagination.ts

@@ -1,6 +1,6 @@
 import {URL} from 'url';
 import test from 'ava';
-import got from '../source';
+import got, {Response} from '../source';
 import withServer, {withBodyParsingServer} from './helpers/with-server';
 import {ExtendedTestServer} from './helpers/types';
 
@@ -406,3 +406,52 @@ test('`stackAllItems` set to false', withServer, async (t, server, got) => {
 
 	t.deepEqual(result, [1, 2, 3]);
 });
+
+test('next url in json response', withServer, async (t, server, got) => {
+	server.get('/', (request, response) => {
+		const parameters = new URLSearchParams(request.url.slice(2));
+		const page = Number(parameters.get('page') ?? 0);
+
+		response.end(JSON.stringify({
+			currentUrl: request.url,
+			next: page < 3 ? `${server.url}/?page=${page + 1}` : undefined
+		}));
+	});
+
+	interface Page {
+		currentUrl: string;
+		next?: string;
+	}
+
+	const all = await got.paginate.all('', {
+		searchParams: {
+			page: 0
+		},
+		responseType: 'json',
+		pagination: {
+			transform: (response: Response<Page>) => {
+				return [response.body.currentUrl];
+			},
+			paginate: (response: Response<Page>) => {
+				const {next} = response.body;
+
+				if (!next) {
+					return false;
+				}
+
+				return {
+					url: next,
+					prefixUrl: '',
+					searchParams: undefined
+				};
+			}
+		}
+	});
+
+	t.deepEqual(all, [
+		'/?page=0',
+		'/?page=1',
+		'/?page=2',
+		'/?page=3'
+	]);
+});