Support RR7 & remix-auth@4 #54
Conversation
|
Great work on this! I am also blocked on migrating until these changes are in so I appreciate the initiative |
|
@pbteja1998 can we get a review on this? 🙏 |
E-Kuerschner
left a comment
E-Kuerschner
left a comment
There was a problem hiding this comment.
Awesome work! Thanks for getting the ball rolling on this
| // remove the magic link and email from the session | ||
| session.unset(this.sessionMagicLinkKey) | ||
| session.unset(this.sessionEmailKey) | ||
| session.unset('magicLink') |
There was a problem hiding this comment.
couple of observations here:
- it looks like the ability to set one's own keys for the cookie storage is no longer available? This feels like a regression in my opinion
- to minimize breaking changes we should leave the default values as is e.g. auth:magiclink
There was a problem hiding this comment.
Still struggling with this one. I just refactored my app to use this set of changes and I had to refactor the shape of my session object. It feels kind of heavy-handed for this auth Strategy to strongly dictate the shape of the Session data
| session.flash(this.sessionErrorKey, { message }) | ||
| const cookie = await sessionStorage.commitSession(session) | ||
| throw redirect(options.failureRedirect, { | ||
| session.flash('error', message) |
There was a problem hiding this comment.
any reason you're choosing to flash the message text directly as opposed to within an object?
|
|
||
| // This should only be called in an action if it's used to start the login process | ||
| if (request.method === 'POST') { | ||
| if (!options.successRedirect) { |
There was a problem hiding this comment.
would it not be useful to keep successRedirect as optional? It can be useful to defer the redirect to the app's business logic. In my apps I have the following for my verify loader:
const { email, name, isAdmin } = await auth.authenticate(
"email-link",
request,
{
failureRedirect: "/auth/login",
},
);
const session = await getSession(request.headers.get("Cookie"));
const redirectBackTo = session.get("auth:loginDestination");
// set the session for a logged in/verified guest user
session.set("user", { email, name, isAdmin });
session.set("isGuest", true);
session.set("emailSubscription", true);
session.unset("auth:email");
session.unset("auth:link");
session.unset("auth:loginDestination");
return redirect(redirectBackTo ?? "/", {
headers: {
"Set-Cookie": await commitSession(session),
},
});
There was a problem hiding this comment.
ahh I see now that the options are no longer passed to the authenticate method, but instead must be specified when the instance of the strategy is created. This comment can be resolved.
E-Kuerschner
left a comment
E-Kuerschner
left a comment
There was a problem hiding this comment.
Looking pretty good and is working with my own application! I hope this can be merged. Left a few comments for feedback!
| * developer to correctly work. | ||
| */ | ||
| export type EmailLinkStrategyOptions<User> = { | ||
| export type EmailLinkStrategyOptions<User, SessionData, SessionFlashData> = { |
There was a problem hiding this comment.
what do you think about an optional callback that is called right before the success redirect that allows the user to change the session? In my own app I was taking advantage of the optional successRedirect and handling the redirect myself. This allowed me to update the session with several other bits of information I wanted to assert about the verified user onto the session
| throw redirect(options.failureRedirect, { | ||
| session.flash('error', message) | ||
| const cookie = await this.sessionStorage.commitSession(session) | ||
| throw redirect(this.failureRedirect, { |
There was a problem hiding this comment.
throwing out an idea I am trying on my own branch - what if we unset the email and magicLink session variables in this case? The user would need to restart the flow anyways wouldn't they? I realized I was doing this myself in my /auth/login route (which I use for the failure redirect) so I just pushed it into the lib and it's working good for my use case!
|
Hey @E-Kuerschner I am so sorry as I missed out notifications on this thread. I am glad you took a look at the changes. I am going through your comments. Will get back |
|
I kind of lost the context here. @E-Kuerschner can you push the changes that you think should be done? Let's discuss on them. I am using this provider as is in one of my apps in production and is working great so far. I am sure your changes will make it even better. @pbteja1998 Are you available for review? |
|
@pskd73 no worries! Take a look at my fork and let me know if you have any tbhoughts: |
2 participants
Hello @pbteja1998 !
This package is great and wanted it to port to remix-auth@4 along with React Router 7 (latest Remix). Following are the significant changes to remix-auth@4
remixisreact-router. So Removed allremixdependenciessessionStorageandoptionsis not passed inauthenticatefunction nowsessionStoragein the constructor optionskeysas I am defining the types specificallytsconfig.jsonas perreact-routerspecificationpackage-lock.json