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.

Sign up for GitHub

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

Jump to bottom

Auth-core-redo #121

Merged
merged 18 commits into from
Dec 5, 2024
Merged

Auth-core-redo #121

merged 18 commits into from
Dec 5, 2024

Conversation

dowski
Copy link
Collaborator

@dowski dowski commented Nov 13, 2024

Includes a number of changes to adapt forked @auth/core code from 0.37 for our purposes.

Includes contributions from dowski and sshader.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@vercel Vercel
Copy link

vercel bot commented Nov 13, 2024 •
edited
Loading

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

Name Status Preview Comments Updated (UTC)
convex-auth-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 5, 2024 2:38pm

@dowski dowski requested a review from sshader November 13, 2024 22:34
dowski
dowski commented Nov 13, 2024
View reviewed changes
test/src/main.tsx Outdated Show resolved Hide resolved
sshader
sshader reviewed Nov 14, 2024
View reviewed changes
Copy link
Contributor

@sshader sshader left a comment

Choose a reason for hiding this comment

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

I think there's also https://github.com/get-convex/convex-auth/blob/main/src/server/provider_utils.ts

Once we get something working, would love to see this rebased into commits of straight copies + the manual changes we make on top so any future upgrades will be easier

@dowski
Copy link
Collaborator Author

dowski commented Nov 26, 2024

I think there's also https://github.com/get-convex/convex-auth/blob/main/src/server/provider_utils.ts

Once we get something working, would love to see this rebased into commits of straight copies + the manual changes we make on top so any future upgrades will be easier

Okay, the whole thing is rebased on top of a straight import of the forked versions. We could optionally check that commit in separately if we want the pristine versions in our actual commit history. I have a branch with just that change if we want to do that.

@dowski dowski requested a review from sshader November 26, 2024 21:52
sshader
sshader approved these changes Dec 3, 2024
View reviewed changes
Copy link
Contributor

@sshader sshader left a comment

Choose a reason for hiding this comment

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

If we've done some manual testing, I'm down.

My last request would be to rebase this cleanly so it's straight copy paste from @auth/core followed by all manual changes (and don't squash the commits), but if that's too time consuming down to just merge as is

test/src/main.tsx Outdated Show resolved Hide resolved
@dowski
Copy link
Collaborator Author

dowski commented Dec 3, 2024

Thanks for the review!

My last request would be to rebase this cleanly so it's straight copy paste from @auth/core followed by all manual changes (and don't squash the commits), but if that's too time consuming down to just merge as is

That's why I did in 84c10c7 - maybe what you mean is that should be checked in separately so it's part of the repo history?

@dowski
Copy link
Collaborator Author

dowski commented Dec 4, 2024

My last request would be to rebase this cleanly so it's straight copy paste from @auth/core followed by all manual changes (and don't squash the commits), but if that's too time consuming down to just merge as is

... maybe what you mean is that should be checked in separately so it's part of the repo history?

Okay, that part is now in a separate PR (#130).

@dowski dowski mentioned this pull request Dec 5, 2024
Merged
sshader and others added 18 commits December 5, 2024 09:38
@sshader @dowski
Copy over callback + checks
86ec9a5
@sshader @dowski
Add adapted files for types, providers, and customFetch
8cfa355
@sshader @dowski
Modify checks + callback
994bb54
@sshader @dowski
Strip out some more unused options
6afcc50
@sshader @dowski
Copy over authorizationUrl.ts
88fd39e
@sshader @dowski
wip
a3a722c
@sshader @dowski
wip
7a20735
@dowski
A few changes to get the test code compiling
a25ebdd
@dowski
Debugging
822d2bc
@dowski
Upgrade to @auth/core 0.37
e214fa3 
Includes a number of changes to adapt forked @auth/core code from 0.37 for our purposes.
@dowski
Remove unused import
1e20084
@dowski
A series of fixes to make Sign in with Apple work
ea60c6b
@dowski
Fixes up a few issues
a82df98 
* getAuthorizationUrl and handleOAuth are now passed an actual InternalProvider, instead of some other type cast as any
* The InternalProvider is constructed from a new oAuthConfigToInternalProvider function that replaces the getConfig function - it converts from an OAuthConfig to an InternalProvider
* Adds a couple bits to the InternalProvider type that were always used along with it
@dowski
Remove some debugging I added earlier
f35afd8
@dowski
Stop ignoring the forked Auth.js code
58b72ed
@dowski
Update docs to ref @auth/core 0.37.0
6666bb1
@dowski
Back out verbose logging in test/
f72b605
@dowski
Ignore lint error from forked code
146e2a3
sshader
sshader approved these changes Dec 5, 2024
View reviewed changes
Copy link
Contributor

@sshader sshader left a comment

Choose a reason for hiding this comment

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

Neat!

src/server/oauth/providers/oauth.ts
/** @internal */
/**
* @internal
* @deprecated
Copy link
Contributor

Choose a reason for hiding this comment

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

we probably shouldn't be adding a @deprecated? Potentially from a bad rebase?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh woops, missed this comment. I'll address in a follow-up.

@dowski dowski merged commit 10b4924 into get-convex:main Dec 5, 2024
4 checks passed
@dowski dowski deleted the auth-core-redo branch December 5, 2024 18:50
@dowski dowski mentioned this pull request Dec 10, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sshader sshader sshader approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@dowski @sshader