Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
2 tasks done
Tracked by #1684
asomethings opened this issue Feb 16, 2021 · 4 comments
Closed
2 tasks done
Tracked by #1684
Labels
bug Something does not work as it should ✭ help wanted ✭

Comments

@asomethings
Copy link

asomethings commented Feb 16, 2021

Describe the bug

  • Node.js version: 14.15.3
  • OS & version: Ubuntu 20.04.1 LTS

Actual behavior

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

  • searchParams does not exist in request options
  • searchParams is an empty object in request options
  • searchParams is an empty array in request options
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

  • I have read the documentation.
  • I have tried my code with the latest version of Node.js and Got.
@szmarczak szmarczak added bug Something does not work as it should ✭ help wanted ✭ labels Feb 16, 2021
@sindresorhus
Copy link
Owner

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

@szmarczak szmarczak mentioned this issue Mar 21, 2021
71 tasks
@asomethings
Copy link
Author

@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
Copy link
Collaborator

@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:

  • instance(..., {searchParams: undefined}})
  • instance(..., {cookieJar: undefined}})
  • instance(..., {responseType: undefined}})
  • instance(..., {prefixUrl: ''})
  • instance(..., {agent: {http: undefined, https: undefined, http2: undefined}})
  • instance(..., {context: {token: undefined, ...}})
  • instance(..., {httpsOptions: {rejectUnauthorized: undefined, ...}})
  • instance(..., {cacheOptions: {immutableMinTimeToLive: undefined, ...}})
  • instance(..., {headers: {'user-agent': undefined, ...}})
  • instance(..., {timeout: {request: undefined, ...}})

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

  • instance(..., {retry: got.defaults.options.retry})
  • instance(..., {pagination: got.defaults.options.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 szmarczak mentioned this issue Apr 11, 2021
13 tasks
@szmarczak
Copy link
Collaborator

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something does not work as it should ✭ help wanted ✭
Projects
None yet
Development

No branches or pull requests

3 participants