-
Notifications
You must be signed in to change notification settings - Fork 19
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
downgrade userId #138
base: main
Are you sure you want to change the base?
downgrade userId #138
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
commit: |
UserDocument extends Record<string, Value> = DocumentByName< | ||
DataModel, | ||
"users" | ||
>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should just have this be the same as other providers, vs. returning a specific type.. not sure if this is causing the type issues
const { user } = await createAccount(ctx, { | ||
const { account } = await createAccount(ctx, { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one concern is that folks who have made their own providers may be tripped up by createAccount
not returning the user anymore. This would be a breaking change technically
export interface PasswordConfig<DataModel extends GenericDataModel> { | ||
export interface PasswordConfig< | ||
DataModel extends GenericDataModel, | ||
UsersTable extends TableNamesInDataModel<DataModel> = "users", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could just do the UsersDoc
like anonymous to be more flexible, but not sure which is more convenient / types better
// TODO: Ian removed this | ||
user: (await ctx.db.get(existingAccount.userId))!, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't remember why this was added back. I thought it wasn't needed beyond getting the userId, but there's a decent amount of untyped code..
ctx.db.normalizeId("users", existingUserId) === null | ||
) { | ||
throw new Error( | ||
`User ID \`${existingUserId}\` is not in the \`users\` table`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could use a better message about defining your own createOrUpdateUser
to use another table
export type ConvexCredentialsConfig = ConvexCredentialsUserConfig<any> & { | ||
export type ConvexCredentialsConfig< | ||
DataModel extends GenericDataModel = GenericDataModel, | ||
> = ConvexCredentialsUserConfig<DataModel> & { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this type might be why there's now circular types issues..?
the proposal here (based on this PR and chatting in person) is
This makes it possible to change the name of the "users" table by
This also makes the advanced use case of completely customizing how users are stored possible (maybe they're in two tables, it's they're in an external system, etc.) by
As a long-term goal, if all interaction with the users table is developer-mediated then there are no Convex Auth-imposed constraints on the users table. Assumptions about the users table (described here) wouldn't be needed. |
We'll also want to make writing a createOrUpdateUser callback simpler if we do this. |
Just exploring what it'd look like if userId was a string with a few annotations on the edges
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.