sindresorhus / got

🌐 Human-friendly and powerful HTTP request library for Node.js
MIT License
14.27k stars 935 forks source link

default searchParams not properly merged when requesting searchParams is undefined #1610

Closed asomethings closed 3 years ago

asomethings commented 3 years ago

Describe the bug

Actual behavior

Does not merge searchParams when searchParams: undefined making a request. But, sends default searchParams when

import got from 'got'

const gotExtended = got.extend({ searchParams: { query: true }, responseType: 'json' })
gotExtended('http://httpbin.org/get', { searchParams: undefined })
  .then((response) => response.body)
  .then(console.log)

/** Above returns
{
  args: {},
  headers: {
    Accept: 'application/json',
    'Accept-Encoding': 'gzip, deflate, br',
    Host: 'httpbin.org',
    'User-Agent': 'got (https://github.com/sindresorhus/got)',
    'X-Amzn-Trace-Id': 'Root=1-602bd8d5-5623f1de6a109692250b3177'
  },
  origin: 'xxx.xxx.xx.xxx',
  url: 'http://httpbin.org/get'
}
*/

Expected behavior

Should pass default searchParams even though requested searchParams is undefined

import got from 'got'

const gotExtended = got.extend({ searchParams: { query: true }, responseType: 'json' })
gotExtended('http://httpbin.org/get', { searchParams: undefined })
  .then((response) => response.body)
  .then(console.log)

/** Above should return
{
  args: { query: 'true' },
  headers: {
    Accept: 'application/json',
    'Accept-Encoding': 'gzip, deflate, br',
    Host: 'httpbin.org',
    'User-Agent': 'got (https://github.com/sindresorhus/got)',
    'X-Amzn-Trace-Id': 'Root=1-602bd8d5-5623f1de6a109692250b3177'
  },
  origin: 'xxx.xxx.xx.xxx',
  url: 'http://httpbin.org/get?query=true'
}
*/

Code to reproduce

import got from 'got'

const gotExtended = got.extend({ searchParams: { query: true }, responseType: 'json' })
gotExtended('http://httpbin.org/get', { searchParams: undefined })
  .then((response) => response.body)
  .then(console.log)

Checklist

sindresorhus commented 3 years ago

Would you be able to submit a pull request with one or more failing tests? That would help to get this fixed faster.

asomethings commented 3 years ago

@sindresorhus Sorry for the late reply. I just found out that it seems to be working as intended referring to normalize-arguments.ts test. It seems the test intends to reset the searchParams by putting undefined value in it. Can you confirm if it's intended behaviour?

szmarczak commented 3 years ago

@asomethings It could be intended behavior, but I still don't like it as it's ambiguous. Anyway #1667 (merged, yay!) is much more stricter when providing options. In order to retain the previous options, simply omit them (or, if defined, use the delete operator).

Currently, only these options accept undefined: searchParams, cookieJar and responseType.

The following will reset those values:

In order to reset other options such as retry and pagination:

Note that instance(..., {hooks: got.defaults.options.hooks}) has no effect. There are two ways to reset hooks:

const createEmptyHooks = () => {
    return {
            init: [],
            beforeRequest: [],
            beforeError: [],
            beforeRedirect: [],
            beforeRetry: [],
            afterResponse: []
    };
};

// 1.
const secondInstance = instance.extend({mutableDefaults: true});
secondInstance.defaults.options.hooks = emptyHooks;

await secondInstance(...);

// 2.
await instance(..., {
    hooks: {
        init: [
              (options, self) => {
                  self.hooks = createEmptyHooks();
              }
        ]
    }
});

We could implement instance(..., {hooks: undefined}), but you may ask how to reset particular hooks? Well, you need to use the above. So hooks doesn't accept undefined in order to be more transparent.

szmarczak commented 3 years ago

Note that the above comment refers to the upcoming version of Got (12).