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

fix error when passing an undefined value #206

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

kudarap
Copy link

@kudarap kudarap commented May 11, 2016

No description provided.

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@kudarap
Copy link
Author

kudarap commented May 11, 2016

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

when there's no passed param on _wwwFormUrlEncodePiece
@e111077
Copy link
Contributor

e111077 commented Jul 13, 2016

Heya, @javinc, thanks for the PR and sorry for the late review.

I see that this can run into issues if the key or the value is undefined, but can you provide a strong use case where this is necessary? An issue that I have with this is that you may have a url with bad params that fails silently.

Additionally, I think that an undefined value to an existent key is best to be kept out of the params as
key: undefined should not be represented as an empty string. Perhaps consider simply skipping the key/value encoding if the value is not defined.

@e111077
Copy link
Contributor

e111077 commented Jul 13, 2016

see #207 null typically means that a value has specifically been set to be empty. undefined typically means that it was never set. I think that the best course of action here is that if it is undefined, that key / value pair should be skipped (e.g. { a: 'asdf', b: undefined } domain?a=asdf) and that a null value should be represented as either domain?key=, domain?key=null, or domain?key=%00.

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

Successfully merging this pull request may close these issues.

3 participants