search

ladjs / superagent

Ajax for Node.js and browsers (JS HTTP client). Maintained for @forwardemail, @ladjs, @spamscanner, @breejs, @cabinjs, and @lassjs.
https://ladjs.github.io/superagent/
MIT License
16.58k stars 1.33k forks source link

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

Closed dsanders11 closed 4 years ago

dsanders11 commented 4 years ago

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

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

niftylettuce commented 4 years ago

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

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

Concide commented 4 years ago

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 commented 4 years ago

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

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

slobo commented 3 years ago

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

dsanders11 commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

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

slobo commented 3 years ago

Thanks guys!

jimmywarting commented 3 years ago

I think URLSearchParams would have been better