Skip to content

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

Closed
acoreyj wants to merge 0 commit into
nextauthjs:mainfrom
acoreyj:main
Closed

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

Conversation

@acoreyj

@acoreyj acoreyj commented Jan 3, 2023

Copy link
Copy Markdown
Contributor

MOVED TO #6767

@vercel

vercel Bot commented Jan 3, 2023

Copy link
Copy Markdown

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

A member of the Team first needs to authorize it.

@vercel

vercel Bot commented Jan 3, 2023

Copy link
Copy Markdown

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)

@balazsorban44 balazsorban44 left a comment

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.

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
Contributor

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.

Comment thread packages/frameworks-remix/package.json Outdated
Comment thread packages/frameworks-remix/package.json Outdated
"@auth/core": "workspace:*",
"@types/node": "^18.7.14",
"next-auth": "workspace:*",
"tsup": "^6.5.0",

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.

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
Contributor

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
Contributor

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
Contributor

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

acoreyj commented Jan 4, 2023

Copy link
Copy Markdown
Contributor Author

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

acoreyj commented Jan 17, 2023

Copy link
Copy Markdown
Contributor Author

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

acoreyj commented Feb 20, 2023

Copy link
Copy Markdown
Contributor Author

@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