Merging searchParams from instance and actual call is buggy or unintuitive

[mirfilip]

Describe the bug

• Node.js version: 14.0.0 but problem is the same on older ones
• OS & version: Mac OSX 12.12.6

Let's say we want to create an instance (using got.extend()) and set some default query params, then be able to pass some as part of the url:

const got = require('got');

const fetch = got.extend({
  prefixUrl: "https://example.org/foo/",
  searchParams: new URLSearchParams({ foo: "bar" }),
  hooks: {
    beforeRequest: [
      ({ url }) => console.log('Making request with url: %s', url)
    ]
  }
});

(async () => {
  const url = "path?opt1=1&opt2=2";
  try {
    const response = await fetch(url);
  } catch (e) {
    console.error(e.message);
  }
})();

Here the same on Repl.it

Actual behavior

Merged searchParams and the whole request URL looks like: https://example.org/foo/path?foo=bar

Expected behavior

I believe it should take all searchParams into consideration, so:
https://example.org/foo/path?foo=bar&opt1=1&opt2=2

I noticed none of docs examples mention passing query params as part of the first param so it's not clear to me whether it's a bug or a feature. Regardless, this works with other HTTP clients in JS ecosystem.

Checklist

• I have read the documentation.
• I have tried my code with the latest version of Node.js and Got.

[Giotino]

It's not a bug.

The reason behind that behavior is that a quuery string (aka. search string) (what comes in an URI after the path between ? and #) it's not necessary a "list" of search params.
URLSearchParams is defined by WHATWG, but the query string can be whatever you want (eg. http://example.com/?randomString).

The only thing I think we could argue is what Got has to keep, the new search string or the old one, the actual behavior is due to the merge of the options and the prevalence of searchParams over the query string https://github.com/sindresorhus/got#url

BTW: if you use searchParams in both cases they should merge.

[mirfilip]

It's not a bug.

The reason behind that behavior is that a quuery string (aka. search string) (what comes in an URI after the path between ? and #) it's not necessary a "list" of search params.
URLSearchParams is defined by WHATWG, but the query string can be whatever you want (eg. http://example.com/?randomString).

The only thing I think we could argue is what Got has to keep, the new search string or the old one, the actual behavior is due to the merge of the options and the prevalence of searchParams over the query string https://github.com/sindresorhus/got#url

BTW: if you use searchParams in both cases they should merge.

Hm, right, ok. The bug/feature discussion then comes down to the design decision whether got should attempt parsing out search params out of query string. I assumed so, so it lead me to believe valid search params would be accepted. It's fairly common, not only in JS ecosystem, to assume it would be the case though.

Thanks.