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

Rename Builder to AuthBuilder and create Builder wrapper #1093

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

speckins
Copy link

@speckins speckins commented Oct 4, 2022

Previously, OmniAuth::Builder was a subclass of Rack::Builder. Rack::Builder#call calls #to_app, rebuilding the Rack application on each request, and, as it says in the Rack::Builder comments, this may have a negative impact on performance.

This commit renames the original OmniAuth::Builder to OmniAuth::AuthBuilder and replaces it with a wrapper to be used as middleware (as is suggested in the documentation).

Using OmniAuth::Builder will now only build the application once.

Resolves #1083.

Previously, OmniAuth::Builder was a subclass of Rack::Builder.
Rack::Builder#call calls #to_app, rebuilding the Rack application on
each request, and, as it says in the Rack::Builder comments, this may
have a negative impact on performance.

This commit renames the original OmniAuth::Builder to OmniAuth::AuthBuilder
and replaces it with a wrapper to be used as middleware (as is suggested
in the documentation).

Using OmniAuth::Builder will now only build the application once.

Resolves omniauth#1083.
@speckins
Copy link
Author

speckins commented Oct 4, 2022

I didn't undef the #call method from AuthBuilder completely as I suggested in the issue; let me know if you want to go that far.

Also, do you think we should add some kind of "signpost" in the Builder code to indicate to downstream users that the class has been renamed? Maybe adding versions of #provider, #options, etc., that raise with a message, or maybe just a comment in the code?

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.

Rack::Builder#to_app is not designed to be called for every request.
1 participant