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

feat: add @auth/astro framework library #9856

Open
wants to merge 35 commits into
base: main
Choose a base branch
from

Conversation

TheOtterlord
Copy link

@TheOtterlord TheOtterlord commented Jan 30, 2024

☕️ Reasoning

This PR is the next iteration of #6463

Introduces an Astro framework package. The package exposes an integration, along with helper methods for both server & client uses.

🧢 Checklist

  • Documentation (Note: The auth-astro.vercel.app autogenerated link doesn't have a deployed site. I'm assuming any deployed example will be hosted by this org)
  • Tests
  • Ready to be merged

🎫 Affected issues

📌 Resources

Copy link

vercel bot commented Jan 30, 2024

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

Name Status Preview Comments Updated (UTC)
auth-docs ❌ Failed (Inspect) Apr 28, 2024 6:16pm
1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
next-auth-docs ⬜️ Ignored (Inspect) Visit Preview Apr 28, 2024 6:16pm

apps/dev/astro/auth.config.ts Outdated Show resolved Hide resolved
packages/frameworks-astro/src/config.ts Outdated Show resolved Hide resolved
Copy link
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! Added some comments!

Besides those, have a look at https://github.com/nextauthjs/next-auth/blob/main/packages/utils/scripts/providers.js

Used eg. here:

"build": "pnpm clean && pnpm providers && tsc",
"clean": "rm -rf *.js *.d.ts* lib providers",
"dev": "pnpm providers && tsc -w",
"test": "vitest -c ../utils/vitest.config.ts",
"providers": "node ../utils/scripts/providers"

This way, users can import providers from @auth/astro/providers/* instead of having to install @auth/core!

Further, for other integrations support automatic environment variable inference by using the setEnvDefaults method like so

setEnvDefaults(process.env, config)

there is also createActionURL, a method from core that helps you create the url for the Request of Auth in a much less hacky way!

And lastly, the default basePath value is expected to be /auth, not /api/auth. (NextAuth.js is the only exception, since historically that was the default basePath there and we did not want to make this a breaking change in v5)

Could you incorporate these changes, and then make sure that the dev app and example app is also correctly updated? 🙏 Thanks!

packages/frameworks-astro/module.d.ts Outdated Show resolved Hide resolved
packages/frameworks-astro/package.json Outdated Show resolved Hide resolved
Comment on lines 13 to 17
".": "./src/index.ts",
"./config": "./src/config.ts",
"./client": "./src/client.ts",
"./server": "./src/server.ts",
"./middleware": "./middleware.ts"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
".": "./src/index.ts",
"./config": "./src/config.ts",
"./client": "./src/client.ts",
"./server": "./src/server.ts",
"./middleware": "./middleware.ts"
".": {
"types": "./index.d.ts",
"import": "./index.js"
},
"./config": {
"types": "./config.d.ts",
"import": "./config.js"
},
"./client": {
"types": "./client.d.ts",
"import": "./client.js"
},
"./server": {
"types": "./server.d.ts",
"import": "./server.js"
},
"./middleware": {
"types": "./middleware.d.ts",
"import": "./middleware.js"
},
"./adapters": {
"types": "./adapters.d.ts"
},
"./providers/*": {
"types": "./providers/*.d.ts",
"import": "./providers/*.js"
}

Copy link
Author

Choose a reason for hiding this comment

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

The Astro package doesn't require bundling, as all Astro projects (regardless of config) support TypeScript files. Is there still a reason to specify both types and import if they both lead to the same file?

packages/frameworks-astro/package.json Outdated Show resolved Hide resolved
packages/frameworks-astro/tsconfig.json Outdated Show resolved Hide resolved
packages/frameworks-astro/src/server.ts Outdated Show resolved Hide resolved
Co-authored-by: Balázs Orbán <info@balazsorban.com>
@NuroDev
Copy link

NuroDev commented Mar 10, 2024

Is there any further things blocking this from getting merged? I'd love to start using it

@ndom91
Copy link
Member

ndom91 commented Mar 11, 2024

There seem to be some merge conflicts. Otherwise it should be close. Needs another last going over, testing, etc.

@TheOtterlord
Copy link
Author

@ndom91 happy to resolve the conflict, seems to just be the pnpm-lock, so I'll try to do this tonight. Also, I have some notes from the last review in Discord that might be relevant when going over for another round of reviewing.

I also know there's been work on the framework guidelines? I've not been able to keep up as much as I wanted, so let me know if I'm missing anything important from those!

@ndom91
Copy link
Member

ndom91 commented Mar 13, 2024

@ndom91 happy to resolve the conflict, seems to just be the pnpm-lock, so I'll try to do this tonight. Also, I have some notes from the last review in Discord that might be relevant when going over for another round of reviewing.

I also know there's been work on the framework guidelines? I've not been able to keep up as much as I wanted, so let me know if I'm missing anything important from those!

Awesome, thanks! Yeah no I don't think there were any changes there. There's just this creating a framework pkg guidelines 👍

Copy link

vercel bot commented Mar 13, 2024

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

A member of the Team first needs to authorize it.

@ndom91
Copy link
Member

ndom91 commented Apr 26, 2024

Hey looking over this and adding some minor things here and there, hope thats okay with you! We shipped a whole new docs site recently, so I reverted your docs/* changes and will add them back to the new docs as soon as I can merge the latest main changes back into your branch. Having trouble with this pnpm-lokc.yaml merge conflict still

Also, I'm curious about the build process for the main @auth/astro package. There doesn't seem to be any. We can't just ship typescript to the client, right.

Also it'd be great to have some tests in here 🙏

@ndom91
Copy link
Member

ndom91 commented Apr 26, 2024

Okay so I added a build step, it creates all .js and .d.ts type files into dist/*, including copies of all providers after copying them into here first.

Also I updated the export map and files array to correctly publish the right files.

Finally, I also removed the api/[...auth].ts file you had that looked like the next-auth dynamic route handler as well. This was mistakenly kept in the framework-astro/ directory, right? Instead of one of the development / example apps? It didn't seem like it belonged here at all, but I'm not 100% sure.

Anyway, going forward, there's a few things that immediately jumepd out at me.

  1. We still have the pnpm-lock.yaml merge conflict. It's got ~350 conflicts, so doing it manually in the GitHub web editor seems undoable 😅 If you have a quick way to take care of that, that'd be amazing. Once that's fixed and the updates from main are merged back in, I'll put all the relevant new docs stuff back 👍
  2. There's still 1 typescript issue when building. In the src/index.ts:162 the virtualConfigModule method has some error. I didn't look into it too deeply, but it'd be great if you cuold if you have time!
  3. Also like I mentioned previously, some tests here would be amazing

Thanks!

@TheOtterlord
Copy link
Author

I didn't add a build step as any Astro project supports TypeScript files out-the-box. I'll leave it as-is unless you'd like to revert it (there's no issue with having a build step, it's just not required).

No, [...auth].ts is actually injected by the Astro integration 😅
It adds the necessary routes without a user needing to create what is essentially a file with 2 lines of code. This behaviour can be disabled for more advanced uses, but it's handled by default when you install the integration, simplifying the setup process. It also doesn't matter if this ends up as ts/js. I'll add this back in to fix it.

  1. Yep, I'll fix the lockfile 👍
  2. Looks like it's just warning that Vite is out of date (and therefore possibly the type). I've updated locally and it looks fine.
  3. Tests sound like a good idea. I won't add those immediately, but I'll try my best to look at how you structure tests this weekend.

Let me know if I can clarify anything, or if you have any questions!

@TheOtterlord
Copy link
Author

@ndom91 Fixed the lockfile, typescript error, and restored the api route. I've yet to look at tests but this should unblock you on docs.

@ndom91
Copy link
Member

ndom91 commented Apr 28, 2024

Looks like we've got a pnpm-lock.yaml merge conflict again 😂

After you fix it, if you could pull in the updates from main to get all the new docs stuff, I can go from there

@TheOtterlord
Copy link
Author

Fixed and up-to-date!

@Apexal
Copy link

Apexal commented May 20, 2024

Any movement on this? Happy to assist in any way as I'm using https://github.com/nowaythatworked/auth-astro

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants