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

Add encoding uri option #185

Conversation

stephanebachelier
Copy link

This add the URI encoding as an opt-in by default tofalse.

What changes:

  • update needle to support encode_uri option
  • added two tests to confirm that it works
  • updated README.

This shoud fixed #183 and I confirm that it fixes alexanderGugel/ied#164

@tomas
Copy link
Owner

tomas commented Aug 17, 2016

Thanks for the PR @stephanebachelier. Everything looks good but I'm thinking we should remove encodeURI altogether and let the developer encode the URI if he wants to. I mean, rather than:

var uri = 'http://some.uri.com/with?some=parameters';
var stream = needle.get(uri, { encode_uri: true });

It feels clearer this way:

var uri = encodeURI('http://some.uri.com/with?some=parameters');
var stream = needle.get(uri);

And it's actually less code. Not to mention one less option to worry about. :)

@kfitzgerald
Copy link
Contributor

@tomas Yes, #171 should be totally rolled back, as it breaks properly encoded urls by double encoding them. If the developer is trying to fetch a broken URL, that's their problem, not needle's.

As far as I'm concerned, v1.1.0 is very broken and is unusable by us, as it breaks our app. :(

@tomas
Copy link
Owner

tomas commented Sep 2, 2016

@kfitzgerald yup, thanks for the heads up. Will revert it in a sec.

@tomas tomas closed this Sep 2, 2016
@tomas
Copy link
Owner

tomas commented Sep 2, 2016

Ok, just pushed 1.1.1 with urlencoding removed. Thanks again guys.

@kfitzgerald
Copy link
Contributor

@tomas Thanks again! Just so happened I simultaneously pushed / made #186, which has that fix and tests associated.

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.

caret installs broken GET requests to signed URLs have started failing
3 participants