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

Should we memoize #OmniAuth::Strategy#credentials? #999

Open
zinosama opened this issue Apr 25, 2020 · 1 comment
Open

Should we memoize #OmniAuth::Strategy#credentials? #999

zinosama opened this issue Apr 25, 2020 · 1 comment

Comments

@zinosama
Copy link
Contributor

zinosama commented Apr 25, 2020

Expected Behavior

#credentials to be called once per callback request.

Actual Behavior

#credentials is called twice per callback request.

Question

Is there any reason OmniAuth::Strategy#credentials is not memoized given that OmniAuth::Strategy#auth_hash always invokes it twice in a row:

# OmniAuth::Strategy#auth_hash 
hash.credentials = credentials if credentials

This results in all providers' credentials block to be evaluated twice per callback request.

I'm happy to open a fix for this if maintainers are open to it. Thoughts? @BobbyMcWho

@zinosama zinosama changed the title Do we want to memoize #OmniAuth::Strategy#credentials? Should we memoize #OmniAuth::Strategy#credentials? Apr 25, 2020
@BobbyMcWho
Copy link
Member

I'm open to a PR, I'm not intimate with that section of code in particular

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

No branches or pull requests

2 participants