next-auth
next-auth copied to clipboard
feat: add @auth/astro framework library
☕️ Reasoning
This PR is the next iteration of https://github.com/nextauthjs/next-auth/pull/6463
Introduces an Astro framework package. The package exposes an integration, along with helper methods for both server & client uses.
🧢 Checklist
- [x] 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
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 |
Is there any further things blocking this from getting merged? I'd love to start using it
There seem to be some merge conflicts. Otherwise it should be close. Needs another last going over, testing, etc.
@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 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 :+1:
@TheOtterlord is attempting to deploy a commit to the authjs Team on Vercel.
A member of the Team first needs to authorize it.
New and removed dependencies detected. Learn more about Socket for GitHub ↗︎
🚮 Removed packages: npm/@solid-auth/[email protected], npm/@solidjs/[email protected], npm/@solidjs/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected]
🚨 Potential security issues detected. Learn more about Socket for GitHub ↗︎
To accept the risk, merge this PR and you will not be notified again.
Alert | Package | Note | Source |
---|---|---|---|
Install scripts | npm/[email protected] |
| |
Install scripts | npm/[email protected] |
| |
Install scripts | npm/@sveltejs/[email protected] |
|
Next steps
What is an install script?
Install scripts are run when the package is installed. The majority of malware in npm is hidden in install scripts.
Packages should not be running non-essential scripts during install and there are often solutions to problems people solve with install scripts that can be run at publish time instead.
Take a deeper look at the dependency
Take a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev.
Remove the package
If you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency.
Mark a package as acceptable risk
To ignore an alert, reply with a comment starting with @SocketSecurity ignore
followed by a space separated list of ecosystem/package-name@version
specifiers. e.g. @SocketSecurity ignore npm/[email protected]
or ignore all packages with @SocketSecurity ignore-all
@SocketSecurity ignore npm/[email protected]
@SocketSecurity ignore npm/[email protected]
@SocketSecurity ignore npm/@sveltejs/[email protected]
Hmm wait, did I mess up the pnpm lock update somehow :sweat_smile:
Haha idk, I'll check later.
Notes for us for when this is merged though:
- [ ] Add astro.authjs.dev domain
- [ ] Add astro example repo
- [ ] Add github perms to sync example from monorepo to astro example repo
Thanks @Dragate! Fixed a small issue with the protocol, and merged the rest.
Any update?
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 :pray:
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.
- 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 :sweat_smile: If you have a quick way to take care of that, that'd be amazing. Once that's fixed and the updates frommain
are merged back in, I'll put all the relevant new docs stuff back :+1: - There's still 1 typescript issue when building. In the
src/index.ts:162
thevirtualConfigModule
method has some error. I didn't look into it too deeply, but it'd be great if you cuold if you have time! - Also like I mentioned previously, some tests here would be amazing
Thanks!
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 :sweat_smile:
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.
- Yep, I'll fix the lockfile :+1:
- 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.
- 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!
@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.
Looks like we've got a pnpm-lock.yaml
merge conflict again :joy:
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
Fixed and up-to-date!
Any movement on this? Happy to assist in any way as I'm using https://github.com/nowaythatworked/auth-astro