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

Explicitly cast boolean and number values in signature base string #39

Merged
merged 2 commits into from
Dec 7, 2017

Conversation

okcoker
Copy link
Contributor

@okcoker okcoker commented Dec 7, 2017

So I just ran into #9 myself. I think @jmendiara has it a little mixed up in his example. Because of this line, if you were to generate a signature with parameters such as the following:

var params = {
     from: 0,
     another: false,
     to: 1
};

The signature base string would include something like: http://foo.com/bar?from=&another=&to=1 instead of http://foo.com/bar?from=0&another=false&to=1.

Without this, the user of the library would have to be mindful in always converting parameter values to strings. I also added tests so this fixes #9.

Any plans on upgrading uri-js for #34 #35 #38?

@okcoker okcoker changed the title Allow false and 0 to be added to base string properly Allow boolean and number values to be added to signature base string Dec 7, 2017
@okcoker okcoker changed the title Allow boolean and number values to be added to signature base string Explicitly cast boolean and number values in signature base string Dec 7, 2017
@bettiolo
Copy link
Owner

bettiolo commented Dec 7, 2017

Makes sense, thanks for implementing the tests.

@bettiolo bettiolo merged commit 509c3b1 into bettiolo:master Dec 7, 2017
@bettiolo
Copy link
Owner

bettiolo commented Dec 7, 2017

Release the updated npm package.

@okcoker
Copy link
Contributor Author

okcoker commented Dec 7, 2017

Thanks for the quick turnaround!

@bettiolo
Copy link
Owner

bettiolo commented Dec 11, 2017

Your PR was awesome, thanks for the effort and for updating the tests

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.

2 participants