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

Include profile on signIn events #2354

Closed
wants to merge 1 commit into from
Closed

Include profile on signIn events #2354

wants to merge 1 commit into from

Conversation

t3dotgg
Copy link
Contributor

@t3dotgg t3dotgg commented Jul 11, 2021

profile added to event.signIn message

Reasoning 💡

I use the profile information to fire updates on the user object after any successful sign-in. Before this change, I had to stuff handlers to sync profile.name and platform against the User model in 3 separate places. This change allows me to have a complete "source of truth" within the signIn event :)

Checklist 🧢

To check an item, place an x in the box like so: - [x] Documentation. -->

  • Documentation (likely needed, will add if y'all like the changes)
  • Tests (not sure if needed for this)
  • Ready to be merged

I would like some thoughts from the team on this solution before cleanup + merge. I am happy to make any docs changes and add tests if this is a solution y'all are happy with :)

Affected issues 🎟

Didn't file one, went straight to source :)

@vercel
Copy link

vercel bot commented Jul 11, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/nextauthjs/next-auth/21ReaC5QHseqMTWtcGsxN39LMtmP
✅ Preview: https://next-auth-git-fork-theobr-main-nextauthjs.vercel.app

@vercel vercel bot temporarily deployed to Preview July 11, 2021 22:51 Inactive
@github-actions github-actions bot added core Refers to `@auth/core` TypeScript Issues relating to TypeScript labels Jul 11, 2021
@balazsorban44
Copy link
Member

balazsorban44 commented Jul 11, 2021

Thank you for the PR! I guess it's a legit change, but I'm curious if you need this change because adapters currently only save a few fields in the database on sign-in?

In that case, this might be relevant https://github.com/nextauthjs/adapters/issues/151

In v4, we will try to make it easier to save anything on the User model and document on how to change your DB schema to support that.

This PR is still fine I think, but hopefully your usecase can be handled more easily in the future.

Question is if you would be fine waiting until v4 with this change anyway, because the internal events invocations did recently change on the next branch. No problem to merge as is, I can fix the conflict, otherwise could you rather do this change against the next branch?

We are close to a major release (hopefully this summer) and didn't expect many PRs changing core stuff, so we didn't change the CONTRIBUTING.md either. 🤔

@t3dotgg
Copy link
Contributor Author

t3dotgg commented Jul 11, 2021

Thank you for the PR! I guess it's a legit change, but I'm curious if you need this change because adapters currently only save a few fields in the database?

In that case, this might be relevant nextauthjs/adapters#151

In v4, we will try to make it easier to save anything on the User model and document on how to change your DB schema to support that.

This PR is still fine I think, but hopefully your usecase can be handled more easily in the future.

@balazsorban44 ty so much for the quick reply! That issue is also very much within my interests 👀

Storing custom fields would be helpful, but would likely put us in a place of "stale data" pretty quick (i.e. user changes display name on Twitch)

This solution has been the quickest I've found thusfar. Resulting UX is super clean too IMO - just "re-auth" to update your name. I have a video demo here:

https://twitter.com/t3dotgg/status/1414135514169430017

@t3dotgg
Copy link
Contributor Author

t3dotgg commented Jul 11, 2021

Oh also - I am using this change right now via patch-package so I'm "unblocked". Happy to migrate to a better solution once one is determined/released :)

@balazsorban44
Copy link
Member

Saw the demo, nice! I think this is a fine addition no matter the reason behind it, so if it's fine by you and you could do this against next instead, I'll happily merge it. 😊

@t3dotgg t3dotgg changed the base branch from main to next July 11, 2021 23:52
@t3dotgg t3dotgg changed the base branch from next to main July 11, 2021 23:53
@t3dotgg
Copy link
Contributor Author

t3dotgg commented Jul 11, 2021

Tried switching base branch on this one and it was a mess (looks like the event callback pattern changed, I prefer the new one though 😄 )

New PR here: #2356

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Refers to `@auth/core` TypeScript Issues relating to TypeScript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants