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

Pass req_opts from constructor args to request in oauth2client #363

Closed
wants to merge 1 commit into from

Conversation

abrgr
Copy link

@abrgr abrgr commented Feb 13, 2015

This permits client code to pass options (e.g. proxy) directly to request (the transport library).

@ryanseys
Copy link
Contributor

Could you write some docs on how I would use this?

@ryanseys
Copy link
Contributor

Also, perhaps some simple tests to ensure the parameters are being passed correctly, and makes documenting much easier.

@abrgr
Copy link
Author

abrgr commented Feb 13, 2015

I'll do my best to add the tests later today.

My usage looks like this:

var oauthOpts = {
    req_opts: {
        proxy: 'http:/myproxy:80'
    }
};

var oauth2Client = new OAuth2(
    cfg.google.clientId,
    cfg.google.clientSecret,
    cfg.google.clientUrl,
    oauthOpts
);

//...

Do you want me to add this to any of the markdown docs or just leave it here and in the tests?

@ryanseys
Copy link
Contributor

Oh hmm. Yeah that's a little messy. Can we instead just add support for a proxy value in the oauthOpts object, rather than munging all the opts together?

@abrgr
Copy link
Author

abrgr commented Feb 13, 2015

I liked the idea of being able to set arbitrary options on request because there are a few that I want set on all external API calls. In my case, I'm setting a proxy and timeout. The idea of munging all the opts seemed more in-line with https://github.com/google/google-api-nodejs-client/blob/master/lib/apirequest.js#L179, which allows arbitrary access to the request options.

That said, if you'd rather cherry-pick properties, I'm happy to make that change. Mind if I add proxy and timeout?

@ryanseys
Copy link
Contributor

Yeah, arbitrary request options are available on the API request level but I didn't think about at the transporter level itself. :( In either case req_opts should probably be better named and use camel case to fit with our conventions. Perhaps requestOpts is a more explicit name to consider.

@abrgr
Copy link
Author

abrgr commented Feb 13, 2015

You got it. I'll update it to requestOpts and add some tests.

@JustinBeckwith
Copy link
Contributor

Greetings! And thanks for the contribution. Sadly - this PR has gotten really stale :( We're taking steps to respond to these in a more timely fashion to prevent this from happening in the future. Apologies!

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.

3 participants