Setting searchParams to URLSearchParams both in create() and get() doesn't merge them correctly

[evolverine]

Steps to reproduce (v. 0.27.0, tested in Firefox 110.0.1):

Variant A

const k = ky.create({searchParams: { api: 123}})
k.get('', { searchParams: new URLSearchParams({_limit_: 1}) })

The URL visited is http://localhost:8080/?headers=[object Object], instead of http://localhost:8080/?api=123&_limit_=1.

Variant B

const k = ky.create({searchParams: { api: 456}})
k.get('', { searchParams: new URLSearchParams({_limit_: 1}) })

The URL visited is http://localhost:8080/?api=456&headers=[object Object], instead of http://localhost:8080/?api=456&_limit_=1.

[sindresorhus]

Please try with the latest Ky version.

[Kenneth-Sills]

Part of the original issue here is fixed (Ky 1.2.3, Node 21.6.2):

const client = ky.create({ searchParams: { api: 123 } });

client.get('', { searchParams: { _limit_: 1 } });
    // http://localhost:8080/?api=123&_limit_=1

client.get('', { searchParams: new URLSearchParams({ _limit_: 1 }) })
    // http://localhost:8080/?api=123

So the headers and [object Object] bit for URLSearchParams merging is resolved, but it completely omits the _limit_ if it's not being provided by an object literal. But it gets worse:

const form1 = new FormData();
form1.append('foo', 'bar');
const form2 = new FormData();
form2.append('bar', 'baz');

const client = ky.create({ searchParams: new URLSearchParams({ api: 123 }), body: form1 });
const response = await client.post('', { searchParams: new URLSearchParams({ _limit_: 1 }), body: form2 });

console.log(response.url);
    // Expected: https://localhost:8080/?api=123&_limit_=1
    // Actual: https://localhost:8080/?
console.log(response.text());
    // Expected: Big dump of form data
    // Actual: [object Object]
console.log(response.type);
    // Expected: multipart/form-data
    // text/plain;charset=utf-8

All sorts of wrong.

Really, this is an issue in how deepMerge interacts with non-trivial object types. Things like FormData and URLSearchParams are objects, but they're unable to be used withObject.entries (returning []) and cannot be expanded through the use of { ...form } (which removes their prototype chain). These special cases should be recognized and either:

1. We end the merge with an error and avoid the added complexity entirely. The nuclear "Well... don't do that?" approach.
2. Skip deep merging (some subset of?) non-literal objects and do a shallow override. IMO the best compromise, assuming good documentation?
3. Stop using trivial merge for all objects, distinguish (an allowable subset of?) objects and use a different approach for merging. This could add a fair amount of complexity, and would still need banning some mergers (how would you even merge ArrayBuffer with FormData, for instance).
4. Leave it as undefined behavior; let the documentation tell people not to even try extending anything but json. But undefined behavior is certainly a bad approach, in my opinion.

But despite my recommendation, I'm not familiar enough with the implementation or the maintainer's intended/desired behaviors to really make a judgement call on what's the best approach for a PR. Either way, we'd definitely need to specifically document this behavior for users, and it may be worth adding some tests to the test suite to watch out for regressions in the future. Merging objects can be a bit finicky and occasionally isn't forward compatible (e.g. Lodash not being able to handle Symbol keys in some functions).

[Kenneth-Sills]

@sholladay Do you have any opinions on this? I know we said in #408 that this would be addressed with the update to extends, but since this is about the parameters in the actual request calls themselves, it's still an outstanding issue. And the fact it applies to the actual request data makes it a bit more awkward to reconcile.

With #611 in mind, a fifth option could be using a similar approach here. Optionally, providing some utility functions to users to make it easier for them to merge the various awkward data types.

[sholladay]

While I have grown weary of deepMerge being the default behavior (especially while it is known to be buggy), we should still try to make it "do what it says on the tin". So I don't want it to be undefined behavior or for certain options to be shallow copied while others aren't.

Let's add whatever special handling of URLSearchParams that we need to.

We probably want to do something roughly like this: https://stackoverflow.com/a/66700015

However, to avoid params getting overwritten by the second object, I suppose we need to deep merge both object entries instead of spreading them.