Skip to content
This repository has been archived by the owner on Jul 12, 2019. It is now read-only.

Encode form requests with + instead of %20 by default #52

Merged
merged 1 commit into from
Nov 17, 2017

Conversation

BrunoBernardino
Copy link
Contributor

@BrunoBernardino BrunoBernardino commented Nov 14, 2017

Most apps and the specs (read https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/encodeURIComponent on application/x-www-form-urlencoded) expect a plus sign instead of %20 in this situation.

Tests were also added. This will probably require a breaking release.

Fixes zapier/zapier-platform-cli#161

Thoughts @bcooksey ?

Most apps and the specs (read https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/encodeURIComponent on application/x-www-form-urlencoded) expect a plus sign instead of %20.

Tests were also added. This will probably require a breaking release.
Copy link
Member

@eliangcs eliangcs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't pull this because this looks straightforward enough. My only suggestion is that we should add a test making sure + is not encoded to %2B, which can happen if you do .replace(' ', '+') first then querystring.stringify().

@BrunoBernardino
Copy link
Contributor Author

BrunoBernardino commented Nov 15, 2017

@eliangcs that's a good point. I'm not sure we wouldn't want those converted, though. We're just trying to keep the spaces converted to +, the spec doesn't say anything about not converting +, especially because if we don't do that conversion, there's no way of sending an actual +, then.

Does that make sense, or am I missing something?

@eliangcs
Copy link
Member

eliangcs commented Nov 15, 2017

@BrunoBernardino I just realized that I was confused. You're right: + should be encoded. Otherwise, there's no way to send an actual +. Please ignore my last comment.

@@ -39,8 +39,7 @@ const coerceBody = (req) => {

// auto coerce form if header says so
if (contentType === FORM_TYPE && req.body && !_.isString(req.body)) {
// TODO: use const FormData = require('form-data'); instead?
req.body = querystring.stringify(req.body);
req.body = querystring.stringify(req.body).replace(/%20/g, '+');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sanity check that node doesn't have a built-in lib to do the encoding the way we want, vs having to do this find and replace?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice idea! Unfortunately there isn't, because we want to do it for everything but spaces. The options for that method only include sending a function that's encodeURIComponent by default.

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

Successfully merging this pull request may close these issues.

3 participants