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

Adds oauth1 improvements to support requestTokenSecrets & dynamic urls. #1253

Closed
wants to merge 2 commits into
base: devel
from

Conversation

Projects
None yet
3 participants
@RobertLowe
Contributor

RobertLowe commented Jul 27, 2013

RobertLowe referenced this pull request Jul 27, 2013

estark37
Add server static assets and an API for retrieving them.
Server assets can be included in a bundle by putting them in the private/
directory of an application, or by registering a build plugin that calls
compileStep.addAsset with a server file. The Assets API (Assets.getText and
Assets.getBinary) allows an application or package to retrieve the contents of
its own server assets.
@timhaines

This comment has been minimized.

timhaines commented on packages/oauth1/oauth1_binding.js in 2cdc2e5 Aug 30, 2013

Would be good to document what some of the common ones are. The consumerKey and secret are mandatory right?

This comment has been minimized.

Owner

RobertLowe replied Aug 30, 2013

Yea, they are (consumerKey and secret). It would be good to document them, I'll have a patch up tomorrow.

Here's an example of the trello-accounts config: https://github.com/RobertLowe/meteor-accounts-trello#additional-configuration

The extra stuff is trello specific and used in building urls.

@timhaines

This comment has been minimized.

timhaines commented on packages/oauth1/oauth1_binding.js in 2cdc2e5 Aug 30, 2013

Was this argument signature change necessary to implement what you wanted (requestTokenSecrets & dynamic urls) because it is breaking..

I took a quick look and can't see a relation between config and requestTokenSecrets/dynamic urls, but may well have missed it.

This comment has been minimized.

Owner

RobertLowe replied Aug 30, 2013

Yes, so you can pluck config data in building urls.

https://github.com/RobertLowe/meteor-accounts-trello/blob/master/trello_server.js#L8-L24

Tests / Twitter / Trello Accounts all worked when I submitted the pull, can you give me some more information?

This comment has been minimized.

timhaines replied Aug 30, 2013

Accounts-twitter works because you updated the oauth1_server code too. I have app code that creates an instance of Oauth1Binding directly (for use against the Twitter api), that breaks from this change. It's not a big deal for me to fix it if your PR is merged, but it may be more of a nuisance for others (not sure). This could be implemented in a non breaking way by adding an options arg at the end, but I'm not set on that, just suggesting it as a possibility.

This comment has been minimized.

Owner

RobertLowe replied Aug 30, 2013

Valid point, didn't think of it.

Do you think it would be a good idea to detect the invocation style and handle accordingly?

Otherwise you'll always need to call it:

new Oauth1Binding(config.consumerKey, config.secret, urls, config)

Which seems a bit awkward. I'm not against it. But I'm also not a fan of increasing complexity by doing magic to detect the invocation style. Perhaps there is a simpler change.

This comment has been minimized.

timhaines replied Aug 30, 2013

Yeah, I don't have a strong preference. @n1mmy - any preference?

This comment has been minimized.

timhaines replied Aug 30, 2013

Actually, I have a preference for you making the breaking change that you've got in this commit. :-P

@RobertLowe

This comment has been minimized.

Contributor

RobertLowe commented Aug 30, 2013

@timhaines ping, added documentation for OAuth1Binding, cheers.

@timhaines

This comment has been minimized.

Contributor

timhaines commented Aug 30, 2013

Cool.

@n1mmy

This comment has been minimized.

Member

n1mmy commented Sep 27, 2013

Thanks, @RobertLowe! Merged in 1582372.

@n1mmy n1mmy closed this Sep 27, 2013

mquandalle pushed a commit to mquandalle/meteor-accounts that referenced this pull request Mar 31, 2016

mquandalle pushed a commit to mquandalle/meteor-accounts that referenced this pull request Mar 31, 2016

mquandalle pushed a commit to mquandalle/meteor-accounts that referenced this pull request Mar 31, 2016

mquandalle pushed a commit to mquandalle/meteor-accounts that referenced this pull request Mar 31, 2016

mquandalle pushed a commit to mquandalle/meteor-accounts that referenced this pull request Mar 31, 2016

mquandalle pushed a commit to mquandalle/meteor-accounts that referenced this pull request Mar 31, 2016

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