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

bug(recorder): replace qs lib with native querystring #1653

Merged
merged 1 commit into from
Aug 6, 2019

Conversation

mastermatt
Copy link
Member

The qs dependency was removed, however, recorder.js retained usage of it.
Replacing it with querystring from the std lib is sufficient.

Introduced: #1632
Fixes: #1651

Note that the output of generateRequestAndResponse will change for requests with search params, however, the net effect when that output is used will be the same.

The `qs` dependency was removed, however, recorder.js retained usage
of it. Replacing it with `querystring` from the std lib is sufficient.

Introduced: nock#1632
Fixes: nock#1651
@paulmelnikow
Copy link
Member

Wouldn't this break recordings created in the old version?

@mastermatt
Copy link
Member Author

I don't think so, but I'll add a test using a recording from v10.

@mastermatt
Copy link
Member Author

This change doesn't affect the way recordings are processed, just recorded when output_objects is not set to true.

example:

got('https://example.com/todos/1?foo[bar]=123&baz[0]=a&baz[1]=b')

// output before change
nock('https://example.com:443', {"encodedQueryParams":true})
  .get('/todos/1')
  .query({"foo":{"bar":"123"},"baz":["a","b"]})
  ...

// output after change
nock('https://example.com:443', {"encodedQueryParams":true})
  .get('/todos/1')
  .query({"foo%5Bbar%5D":"123","baz%5B0%5D":"a","baz%5B1%5D":"b"})
  ...

And we already have tests in test_complex_querystring.js showing that both styles are able to be processed.

@paulmelnikow
Copy link
Member

Thanks for looking into that!

@mastermatt mastermatt merged commit e661d0d into nock:beta Aug 6, 2019
@mastermatt mastermatt deleted the fix-qs-in-recorder branch August 6, 2019 12:09
@nockbot
Copy link
Collaborator

nockbot commented Aug 12, 2019

🎉 This PR is included in version 11.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

gr2m pushed a commit that referenced this pull request Sep 4, 2019
The `qs` dependency was removed, however, recorder.js retained usage
of it. Replacing it with `querystring` from the std lib is sufficient.

Introduced: #1632
Fixes: #1651
gr2m pushed a commit that referenced this pull request Sep 4, 2019
The `qs` dependency was removed, however, recorder.js retained usage
of it. Replacing it with `querystring` from the std lib is sufficient.

Introduced: #1632
Fixes: #1651
gr2m pushed a commit that referenced this pull request Sep 5, 2019
The `qs` dependency was removed, however, recorder.js retained usage
of it. Replacing it with `querystring` from the std lib is sufficient.

Introduced: #1632
Fixes: #1651
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants