Fix options.searchParams duplicates

Fixes #1188

Author: szmarczak

source/core/index.ts

@@ -560,21 +560,37 @@ 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 as NormalizedOptions, ...(url as Options), ...options};
+			options = {...defaults, ...(url as Options), ...options};
 		} else {
 			if (url && options && options.url) {
 				throw new TypeError('The `url` option is mutually exclusive with the `input` argument');
 			}
 
-			options = {...defaults as NormalizedOptions, ...options};
+			options = {...defaults, ...options};
 
 			if (url) {
 				options.url = url;
 			}
 		}
 
+		// 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).
@@ -637,6 +653,28 @@ export default class Request extends Duplex implements RequestEvents<Request> {
 			throw new TypeError('Parameter `auth` is deprecated. Use `username` / `password` instead.');
 		}
 
+		// `options.searchParams`
+		if (options.searchParams) {
+			if (!is.string(options.searchParams) && !(options.searchParams instanceof URLSearchParams)) {
+				validateSearchParameters(options.searchParams);
+			}
+
+			options.searchParams = new URLSearchParams(options.searchParams as Record<string, string>);
+
+			// `normalizeArguments()` is also used to merge options
+			if (defaults?.searchParams) {
+				defaults.searchParams.forEach((value, key) => {
+					(options!.searchParams as URLSearchParams).append(key, value);
+				});
+			}
+		} else {
+			options.searchParams = defaults?.searchParams;
+		}
+
+		// `options.username` & `options.password`
+		options.username = options.username ?? '';
+		options.password = options.password ?? '';
+
 		// `options.prefixUrl` & `options.url`
 		if (options.prefixUrl) {
 			options.prefixUrl = options.prefixUrl.toString();
@@ -675,7 +713,7 @@ export default class Request extends Duplex implements RequestEvents<Request> {
 				get: () => prefixUrl
 			});
 
-			// Protocol check
+			// Support UNIX sockets
 			let {protocol} = options.url;
 
 			if (protocol === 'unix:') {
@@ -684,23 +722,25 @@ export default class Request extends Duplex implements RequestEvents<Request> {
 				options.url = new URL(`http://unix${options.url.pathname}${options.url.search}`);
 			}
 
+			// Set search params
+			if (options.searchParams) {
+				options.url.search = options.searchParams.toString();
+			}
+
+			// Trigger search params normalization
 			if (options.url.search) {
 				const triggerSearchParameters = '_GOT_INTERNAL_TRIGGER_NORMALIZATION';
 
 				options.url.searchParams.append(triggerSearchParameters, '');
 				options.url.searchParams.delete(triggerSearchParameters);
 			}
 
+			// Protocol check
 			if (protocol !== 'http:' && protocol !== 'https:') {
 				throw new UnsupportedProtocolError(options as NormalizedOptions);
 			}
-		}
 
-		// `options.username` & `options.password`
-		options.username = options.username ?? '';
-		options.password = options.password ?? '';
-
-		if (options.url) {
+			// Update `username` & `password`
 			options.url.username = options.username;
 			options.url.password = options.password;
 		}
@@ -725,27 +765,6 @@ 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);
-			}
-
-			options.searchParams = new URLSearchParams(options.searchParams as Record<string, string>);
-
-			// `normalizeArguments()` is also used to merge options
-			const defaultsAsOptions = defaults as Options | undefined;
-			if (defaultsAsOptions && defaultsAsOptions.searchParams instanceof URLSearchParams) {
-				defaultsAsOptions.searchParams.forEach((value, key) => {
-					(options!.searchParams as URLSearchParams).append(key, value);
-				});
-			}
-
-			if (options.url) {
-				options.url.search = options.searchParams.toString();
-			}
-		}
-
 		// `options.cache`
 		const {cache} = options;
 		if (cache) {
@@ -782,7 +801,6 @@ export default class Request extends Duplex implements RequestEvents<Request> {
 		}
 
 		// `options.hooks`
-		const areHooksUserDefined = options.hooks !== defaults?.hooks;
 		options.hooks = {...options.hooks};
 
 		for (const event of knownHookEvents) {
@@ -798,7 +816,7 @@ export default class Request extends Duplex implements RequestEvents<Request> {
 			}
 		}
 
-		if (defaults && areHooksUserDefined) {
+		if (defaults) {
 			for (const event of knownHookEvents) {
 				const defaultHooks = defaults.hooks[event];
 

test/arguments.ts

@@ -138,6 +138,18 @@ test('overrides `searchParams` from options', withServer, async (t, server, got)
 	t.is(body, '/?test=wow');
 });
 
+test('does not duplicate `searchParams`', withServer, async (t, server, got) => {
+	server.get('/', echoUrl);
+
+	const instance = got.extend({
+		searchParams: new URLSearchParams({foo: '123'})
+	});
+
+	const body = await instance('?bar=456').text();
+
+	t.is(body, '/?foo=123');
+});
+
 test('escapes `searchParams` parameter values', withServer, async (t, server, got) => {
 	server.get('/', echoUrl);
 

test/error.ts

@@ -178,7 +178,7 @@ test('`http.request` error through CacheableRequest', async t => {
 	});
 });
 
-test('errors are thrown directly when options.stream is true', t => {
+test('errors are thrown directly when options.isStream is true', t => {
 	t.throws(() => {
 		// @ts-ignore Error tests
 		got('https://example.com', {isStream: true, hooks: false});