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

fix: use qs to serialize form data in browser #1591

Merged
merged 1 commit into from
Aug 30, 2020

Conversation

dsanders11
Copy link
Contributor

Unifies behavior for Node and browser with regards to 'application/x-www-form-urlencoded'

Previous PR was #1589, but this is a better fix.

Unifies behavior for Node and browser with regards to  'application/x-www-form-urlencoded'
@niftylettuce
Copy link
Collaborator

v6.1.0 has been released to npm, thank you @dsanders11

https://github.com/visionmedia/superagent/releases/tag/v6.1.0

@Concide
Copy link

Concide commented Aug 31, 2020

Are you sure that this can't be fixed without qs? This PR increased package size by 33.4%

https://bundlephobia.com/result?p=superagent@6.1.0

@niftylettuce
Copy link
Collaborator

This may be a better alternative, I am open to a PR.

https://github.com/sindresorhus/query-string

@slobo
Copy link
Contributor

slobo commented Dec 28, 2020

As implemented, this also breaks backwards compatibility - see #1611. It would be nice if this change in behaviour was at least documented.

@dsanders11
Copy link
Contributor Author

@slobo - not sure if "backwards compatibility" is really the correct description here. Breaking change, sure. But the behavior before was different between browser and Node, which was a bug, so one of them was going to change existing behavior to fix that. While its unfortunate, can't retain backwards compatibility when the previous behavior was the result of a bug.

As @niftylettuce said in an earlier comment, a PR for any suggested changes (or documenting the change in behavior) would probably be appreciated.

@slobo
Copy link
Contributor

slobo commented Dec 29, 2020

Thanks @dsanders11 , I appreciate the tough position, and the desire to have browser and Node behave the same.

As a suggestion, this may have warranted a 7.0 release - I believe it was introduced in 6.1*. We have been using supreagent for a number of years on the client, and did not expect a minor update 6.0 > 6.1 to break our browser code.

I've created a PR to document this somewhere visible: #1614

* I may be mistaken and this was a part of 6.0 release, in which case it's semver-y compliant change, albeit still worth of a mention in the readme :)

@dsanders11
Copy link
Contributor Author

@slobo, you're right, I think it was part of v6.1. I think that was @niftylettuce just trying to get the fix into a release in a timely manner. You're right that it probably warranted a v7 to make the breaking nature clear. I think your PR is in the right direction.

@niftylettuce
Copy link
Collaborator

Yeah this can be a breaking change and I'll do a major semver bump

@slobo
Copy link
Contributor

slobo commented Jan 4, 2021

Thanks guys!

@jimmywarting
Copy link
Contributor

I think URLSearchParams would have been better

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants