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

Add a Passwordless::ControllerHelpers#sign_in_authenticatable method #158

Closed
wants to merge 2 commits into from

Conversation

hutchike
Copy link

The old version of Passwordless allowed me to sign in a user with a simple sign_in user call. The new version requires me to pass a session object instead, so I've added a simple sign_in_authenticatable user method to complement it. Hope you like it. I use this when I'm auth'ing via a Google login instead of a sign-in link via email.

@henrikbjorn
Copy link
Contributor

Would it be better to have a create_passwordless_session method, and then have people do sign_in create_passwordless_session(user)

I think that is the better API.

@mikker
Copy link
Owner

mikker commented Aug 21, 2023

I see how it would be nice to have a shortcut sometimes, but I think the explicitness of having to go through building the session is almost a feature as you're forced to recognise how it's actually the session that gets "signed in".

What you suggest @henrikbjorn is awfully close to the already existing build_passwordless_session method. So close that I don't want to add a new api right now.

@mikker mikker closed this Aug 21, 2023
@hutchike
Copy link
Author

Thanks for considering the PR. I just wanted to remind that the original API allowed us to sign_in an authenticatable object (e.g. a user), whereas the new version doesn't, so it's a breaking change that will trip many people up. I'm sure there's a more elegant way to restore the ability to sign in an authenticatable object, and I do think this is a feature that's worth retaining. Just my 2c. Thanks again.

@mikker
Copy link
Owner

mikker commented Aug 21, 2023

Thank you for that note. I think you're right. I'll make sure to mention it in the changelog that it's going away.

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.

None yet

3 participants