Implement rfc3986 option

[simov]

Fixes #578
Fixes #1287

My first try on the rfc3986 issue, @mikeal @nylen @olado let me know what do you think

[simov]

I just enabled the last test simov@e24762f since #1314 got in.

[nylen]

Is there a reason why we wouldn't want this to be the default behavior (would that break things)?

Best way to document this? Probably mention it in the OAuth example.

[simov]

As quoted in this #1287 (comment) there wouldn't be any problems, currently all parameters in the authorization header are being rfc3986 encoded (in the OAuth method).

I opted out for an option due to the breaking changes introduced by some of the new features lately (and because it's used only for Twitter)

As for the end users, they can set this option through the defaults method to use with each request to Twitter.

If it's going to be an option it should be documented under the options section.

[simov]

@nylen I think enabling this by default should be fine, I just made some tests on my own, plus the implementation would be a bit cleaner without all of these conditional statements.

[nylen]

It only matters for *, but according to https://tools.ietf.org/html/rfc5849#page-29 the hex encoding should be upper case:

 function rfc3986 (str) {
   return str.replace(/[!'()*]/g, function(c) {
-    return '%' + c.charCodeAt(0).toString(16)
+    return '%' + c.charCodeAt(0).toString(16).toUpperCase()
   })
 } 

[simov]

Ok, I saw the upper case bits, what about the * symbol ? Also what is your opinion on enabling this encoding by default?

[nylen]

Same issue (the only character in !'()* that has a letter in its hex representation is *).

I'm for enabling this by default, unless someone can tell me that it would break things. What have you done to test it so far?

[simov]

Ok, I can remove the option and just wrap all places where it's used with the rfc3986 encoding method.

The unit tests are here basically testing with a bunch of options combinations. There a few more I think, but at the time I was writing it, I though they are not so important.

I ran my system tests in purest with the code from this PR. I added the !'()* symbols in the post tests, just a couple of providers there, and checked the output on their web interfaces, I was curious if the symbols are printed correctly. Also a few email APIs there all seems to work without a problem, I haven't tested with multipart though.

EDIT: scratch the multipart bits 😁

[nylen]

👍 I think after that we are good to merge.

Since it's the correct behavior according to a couple of RFCs, I don't think we need docs for this. And any decoding implementation shouldn't care which characters are encoded, so I don't see how it will break anything.

A confirmation that this fixes the original issue with Twitter would be nice also? Ping @olado

[olado]

Thank you for looking into this issue @simov @nylen . I can't test it at the moment, will try in the evening.

[simov]

Pushed, made it by default. A confirmation from @olado would be great idd.

Btw, @nylen I have tests for Twitter, my point with my previous comment was that I tested with a bunch of other REST API providers as well. In fact I use the exact same function in my module from about an year ago, but I have an option in my layer to override the request options per provider if needed. So currently I'm using it only for Twitter, my concerns were with the rest of the providers, not Twitter.

[simov]

Doh, forgot to add the upper case bits yesterday.

[nylen]

Ping @olado re Twitter test. Going ahead and merging though, thanks @simov.