logto-io / js

🤓 Logto JS SDKs.
https://docs.logto.io/quick-starts/
MIT License
67 stars 40 forks source link

feat: add support for sign up flow in Remix #723

Closed samos123 closed 5 months ago

samos123 commented 5 months ago

Summary

Add a sign up flow for Remix: https://github.com/logto-io/js/issues/722

Testing

I published an NPM package and using it in production: https://www.npmjs.com/package/@samos123/logto-remix

You can try it with:

npm install @samos123/logto-remix@2.1.10-sam1

Then use the following for auth routes:

import { logto } from "~/services/authentication";

export const loader = logto.handleAuthRoutes({
  "sign-in": {
    path: "/api/logto/sign-in",
    redirectBackTo: "/api/logto/callback", // The redirect URI just entered
  },
  "sign-in-callback": {
    path: "/api/logto/callback",
    redirectBackTo: "/",
  },
  "sign-out": {
    path: "/api/logto/sign-out",
    redirectBackTo: "/",
  },
  "sign-up": {
    path: "/api/logto/sign-up",
    redirectBackTo: "/api/logto/callback", // The redirect URI just entered
  },
});

Checklist

darcyYe commented 5 months ago

Thanks for your contribution! Overall LGTM, better wait for another stamp. BTW, we should not manually update the version of a package. You can run pnpm changeset add and follow the guide to add a changeset. Will approve once the changeset is properly dealt with. If my previous comment brought misunderstanding, I apologize.

samos123 commented 5 months ago

not at all, I am new to the whole JS/TS ecosystem so I wasn't aware about changeset being a thing. I am happy to iterate and learn.

I have pushed the result of running changeset but not sure if I did that right. I have also reverted the version bump in package.json since I'm assuming that will be done automatically once PR is merged?

Could you do another trigger of the CI jobs and review again? Thank you!

samos123 commented 5 months ago

@darcyYe friendly ping on it. I hope to get it merged so I can switch back to upstream logto remix package.

samos123 commented 5 months ago

I have also kept the README changes to only include add support for sign up flow. The current incorrect README can be fixed separately since I don't want it to hold up this PR.

charIeszhao commented 5 months ago

Hmmm, there's one more thing. Can you sign your commits? (Signing commits)

Out Git repo requires all commits to be signed. Although I can co-edit the commit on behalf, it would be better to setup the commit signature on your side in the long run.

samos123 commented 5 months ago

Done. I setup automatic signing. Never did it before, but now all my commits should be signed going forward :+1:

Can you please trigger CI jobs and merge? I didn't change the contents of the PR in any way.

samos123 commented 5 months ago

Weird. Can someone rerun the GitHub action? The failure seems unrelated to my change and likely due to flaky test?

charIeszhao commented 5 months ago

Done. I setup automatic signing. Never did it before, but now all my commits should be signed going forward 👍

Can you please trigger CI jobs and merge? I didn't change the contents of the PR in any way.

No worries. I'll take a look

charIeszhao commented 5 months ago

Seems the rerun works now. WIll merge the PR and release a new version now