Closed ndangles closed 5 years ago
This LGTM.
I don't follow the Node version story, as the docs mention v0.1.99 for encode
and v0.1.25 for stringify
. It seems both have been around forever. In any case, neither is deprecated, so I don't really care which you picked :)
Could you rebase this PR on master?
rebased
@ndangles Have you been able to test this with Google Play?
To me it looks like Google is passing options as null
to lib/https/post.js https://github.com/Wizcorp/node-iap/blob/58089d4cfcb29490cd31770d292288f9a36375ea/lib/google/index.js#L123
Which means it would not hit the code path that I changed because of the if conditional on options.form, correct? https://github.com/Wizcorp/node-iap/blob/58089d4cfcb29490cd31770d292288f9a36375ea/lib/https/post.js#L12
Yeah so it looks like querystring
produces the identical format and value as the current version of form-urlencoded
for Google Play
Test code:
var formUrlencoded = require('form-urlencoded');
var querystring = require('querystring');
var jwtToken = 'eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwibmFtZSI6IkpvaG4gRG9lIiwiaWF0IjoxNTE2MjM5MDIyfQ.SflKxwRJSMeKKF2QT4fwpMeJf36POk6yJV_adQssw5c';
var formData = {
grant_type: 'urn:ietf:params:oauth:grant-type:jwt-bearer',
assertion: jwtToken
}
var f = formUrlencoded.encode(formData);
var q = querystring.stringify(formData);
if(f === q) {
console.log('true');
console.log(f);
console.log(q);
}
Test Results:
true
grant_type=urn%3Aietf%3Aparams%3Aoauth%3Agrant-type%3Ajwt-bearer&assertion=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwibmFtZSI6IkpvaG4gRG9lIiwiaWF0IjoxNTE2MjM5MDIyfQ.SflKxwRJSMeKKF2QT4fwpMeJf36POk6yJV_adQssw5c
grant_type=urn%3Aietf%3Aparams%3Aoauth%3Agrant-type%3Ajwt-bearer&assertion=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwibmFtZSI6IkpvaG4gRG9lIiwiaWF0IjoxNTE2MjM5MDIyfQ.SflKxwRJSMeKKF2QT4fwpMeJf36POk6yJV_adQssw5c
We need a test suite :(
First one to do it, I buy a beer.
haha maybe if I find some extra time on my hands I can take a whack at it, would probably use jest unless there is a specific preference to a different test framework
I love Jest, so that would work perfectly fine for me :)
Currently node-iap is using an old version of form-urlencoded, this PR removes this third party dependency and uses the nodejs builtin querystring which will accomplish the same thing and reduce the outside dependency.
I am specifically using
.stringify()
instead of.encode()
on querystring because encode is simply just an alias for stringify and stringify was the original function name in node so it has backwards compatibility back to Node v4.9.1 according to their docs