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

mergeOptions isn't merging URLSearchParams instances #1011

Closed
2 tasks done
alexghr opened this issue Jan 4, 2020 · 1 comment · Fixed by #1051
Closed
2 tasks done

mergeOptions isn't merging URLSearchParams instances #1011

alexghr opened this issue Jan 4, 2020 · 1 comment · Fixed by #1051
Assignees
Labels
bug Something does not work as it should ✭ help wanted ✭ regression Something does not work anymore

Comments

@alexghr
Copy link
Contributor

alexghr commented Jan 4, 2020

Describe the bug

  • Node.js version: -
  • OS & version: -

got.mergeOptions isn't merging instances of URLSearchParams. According to the README is should merge them.

Actual behavior

It looks like later instances of URLSearchParams just overwrite earlier values. (see code sample)

Expected behavior

It should merge the two URLSearchParams instances.

Code to reproduce

const got = require('got');
got.mergeOptions({ searchParams: new URLSearchParams({ foo: 1 }) }, { searchParams: new URLSearchParams({ bar: 2 }) });
// => { ... searchParams: URLSearchParams { 'bar' => '2' } }

Checklist

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

I think some code got deleted in #921.

merge.ts got moved https://github.com/sindresorhus/got/pull/921/files#diff-5e2f9cb0db248d822e1b167d5ccf8e07. Before the move it had code to deal with instances of URLSearchParams:

got/source/merge.ts

Lines 13 to 21 in 9f4fe33

if (targetValue instanceof URLSearchParams && sourceValue instanceof URLSearchParams) {
const params = new URLSearchParams();
const append = (value: string, key: string): void => params.append(key, value);
targetValue.forEach(append);
sourceValue.forEach(append);
// @ts-ignore https://github.com/microsoft/TypeScript/issues/31661
target[key] = params;

After the move it's missing the code to handle URLSearchParams: https://github.com/szmarczak/got/blob/a7e73f2fe89b1eca00d2f8fff71dffb2caf68c16/source/utils/merge.ts

@sindresorhus sindresorhus added the bug Something does not work as it should label Jan 4, 2020
@szmarczak szmarczak added the regression Something does not work anymore label Jan 4, 2020
@szmarczak szmarczak self-assigned this Jan 4, 2020
@szmarczak
Copy link
Collaborator

There were no tests for that functionality, so I guess I did remove that innocent piece of code.

I'll fix this on Monday, currently I'm not at home.

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 ✭ regression Something does not work anymore
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants