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 warning where request/request compatible code would silently fail #58

Open
naugtur opened this issue Dec 12, 2014 · 6 comments
Open

Comments

@naugtur
Copy link
Owner

naugtur commented Dec 12, 2014

request has some options that would be silently ignored if one tried to use them with xhr, eg.

requestORxhr({
  url: "http://google.com",
  qs: {
    q: "how do I search internet?"
  }
})

request will send the query, but xhr doesn't support qs (if it should is a totally different matter, we keep the size small)

Proposal:
run console.warn(optionKey+' option not implemented')

@reqshark
Copy link
Contributor

ya don't need to add extra overhead of supporting querystring.

if someone wants that they should use a shitty-qs or something like it

@naugtur
Copy link
Owner Author

naugtur commented Dec 12, 2014

I mean the other way around than shitty-qs works, but that's the main point - you can use anything to put query params in the url (I like how Array.join works for that :P)
But this is just an example and I'd like to list all options that exist in request and are not supported, and then throw warnings if someone uses them.
The size increase should not be that bad - bytes for the list + ~3 lines of code.

@Raynos
Copy link
Collaborator

Raynos commented Dec 12, 2014

@naugtur

console warnings break the unix rule of silence.

If you want to warn the user, throw an exception.

@naugtur
Copy link
Owner Author

naugtur commented Dec 12, 2014

Ok, thanks for the reminder.
I always had trouble following the rule of silence in my ideas...

@TehShrike
Copy link
Collaborator

In the browser, I think console warnings are better than giving no indication. I've added that in one of my own libraries and been very glad I did, it's saved me a good amount of debugging time.

Throwing an exception is fine too. It seems like overly-aggressive duck typing in the browser - in the browser I'm more willing to allow warnings instead of exceptions in cases like this - but I would support throwing an error in the case of input options that aren't implemented.

@naugtur
Copy link
Owner Author

naugtur commented Jun 1, 2016

It's a hard decision. Sometimes, when you're reusing code between node and browser and put xhr in place of request you want certain features to only work in node.

I was thinking we could expose a xhr.registerWarningCallback function where you could pass console.warn or anything you like.

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

No branches or pull requests

4 participants