Skip to content
This repository has been archived by the owner on Aug 17, 2019. It is now read-only.

Adding ability to add default options for request. #92

Open
ghost opened this issue Jul 14, 2016 · 4 comments
Open

Adding ability to add default options for request. #92

ghost opened this issue Jul 14, 2016 · 4 comments

Comments

@ghost
Copy link

ghost commented Jul 14, 2016

Sometimes the flickr api takes a lot of time to respond with content. While using heroku for deployment, usually heroku times out a request every 30 seconds, so I would like to timeout the request to the flickr api sometime before the 30 second timeout is reached. Presently for request.get there is no way to add default parameter to request. It would be great to have one, so that I can configure the requests for the flickrapi as well.

ghost pushed a commit to kreeti/node-flickrapi that referenced this issue Jul 14, 2016
@ghost
Copy link
Author

ghost commented Jul 14, 2016

sent a PR #91

@Pomax
Copy link
Owner

Pomax commented Jul 14, 2016

Hm, in this case I'd not make this a two level object, but make that an explicit value in the options, like requestTimeout in the options, and then unpacking that value when giving request the options it needs.

Using request.defaults(options.requestOptions) means that rather than offering a way to do timeouts, we're offering a way to do everything that request.defaults lets you overwrite, in which case we should tell people where the documentation for that is, instead of claiming that there's only one value supported.

Either way, the PR will probably need a bit of a rewrite to either offer a dedicated value for just this specific use, or to handle the request options object as literally that; an object to pass into request.defaults

@ghost
Copy link
Author

ghost commented Jul 15, 2016

If another day someone wants to add another option to the request.defaults, like gzip: true, then (s)he would have to write add that explicitly to this library. So I think letting the user pass the defaults to the request.defaults as is will be better.

@Pomax
Copy link
Owner

Pomax commented Jul 15, 2016

agreed - if you can update the link to the request.default docs specifically I think we can just merge this in.

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

No branches or pull requests

1 participant