Skip to content

@auth/remix framework for using @auth in remix#6270

Closed
acoreyj wants to merge 0 commit intonextauthjs:mainfrom
acoreyj:main
Closed

@auth/remix framework for using @auth in remix#6270
acoreyj wants to merge 0 commit intonextauthjs:mainfrom
acoreyj:main

Conversation

@acoreyj
Copy link
Copy Markdown
Contributor

@acoreyj acoreyj commented Jan 3, 2023

MOVED TO #6767

@vercel
Copy link
Copy Markdown

vercel bot commented Jan 3, 2023

@acoreyj is attempting to deploy a commit to the authjs Team on Vercel.

A member of the Team first needs to authorize it.

@vercel
Copy link
Copy Markdown

vercel bot commented Jan 3, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated
auth-docs ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Jan 24, 2023 at 4:02AM (UTC)
1 Ignored Deployment
Name Status Preview Comments Updated
next-auth-docs ⬜️ Ignored (Inspect) Jan 24, 2023 at 4:02AM (UTC)

Copy link
Copy Markdown
Member

@balazsorban44 balazsorban44 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! As a preliminary review to set some expectations, I would like to point out that we should aim to embrace the framework as much as possible, rather than reusing code from other framework implementations. I'll have to dig deeper into Remix, but I have a feeling that the current client module is not the "Remix-way". We could merge it for now, but will have to improve before hitting a stable release version.

@@ -0,0 +1,16 @@
import path from "path";
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think, this file should be needed.

"@auth/core": "workspace:*",
"@types/node": "^18.7.14",
"next-auth": "workspace:*",
"tsup": "^6.5.0",
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's not add another tool. we already have tsc, let's remove this

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the heads up, we should do the same there too. 👍

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

solid start is now using tsc instead of tsup aswell

cad4b59

@@ -0,0 +1,14 @@
import { defineConfig } from "tsup";
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's remove

@@ -0,0 +1,4 @@
{
"extends": "./tsconfig.json",
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should only have single tsconfig, let's remove this. the eslint config is in the root directory

@acoreyj
Copy link
Copy Markdown
Contributor Author

acoreyj commented Jan 4, 2023

Thanks for the PR! As a preliminary review to set some expectations, I would like to point out that we should aim to embrace the framework as much as possible, rather than reusing code from other framework implementations. I'll have to dig deeper into Remix, but I have a feeling that the current client module is not the "Remix-way". We could merge it for now, but will have to improve before hitting a stable release version.

Hmm good call I think I can adapt to using the createCookieSessionStorage function from remix

@acoreyj
Copy link
Copy Markdown
Contributor Author

acoreyj commented Jan 17, 2023

This is pretty much working but I'm hoping on seeing some bug fixes in upcoming remix releases that this needs so patches aren't needed.

Specifically

remix-run/react-router#9913
remix-run/remix#3330
remix-run/remix#5095

@cliffordfajardo
Copy link
Copy Markdown

@acoreyj - is the auth.js adapter for remix still something you'd want to tackle after those bug fixes you mentioned are resolved? Currently using Remix Auth to get Azure Authentication working, but would love to get it working with Auth.js Remix adapter 😄

@acoreyj
Copy link
Copy Markdown
Contributor Author

acoreyj commented Feb 20, 2023

@cliffordfajardo

Yea I have actually been using it myself by using patch-package to implement the change from the remix PR if that's something you want to do.

See the new MR at #6767 and checkout the README.md in the files changed

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.

4 participants