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: update Firebase adapter to use new API #3873

Merged
merged 3 commits into from Jul 11, 2022
Merged

feat: update Firebase adapter to use new API #3873

merged 3 commits into from Jul 11, 2022

Conversation

chanceaclark
Copy link
Contributor

@chanceaclark chanceaclark commented Feb 7, 2022

Ports and refactors adapter-firebase to use new Adapter API. Ported from this PR: nextauthjs/adapters#183

Breaking Changes:

  • Requires Firebase@9+
  • Adapter now consumes FirebaseOptions
  • Additional emulator options to connect to Firestore emulator

Reasoning 💡

Uses new firebase modular + adapter api.

Checklist 🧢

  • Documentation
  • Tests
  • Ready to be merged

Affected issues 🎟

Closes #3827

cc. @balazsorban44

@vercel
Copy link

vercel bot commented Feb 7, 2022

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/B9LdbAmzy78jXehodwfN8ttCYbf1
✅ Preview: https://next-auth-git-fork-chanceaclark-feat-firebase-c1d8cc-nextauthjs.vercel.app

[Deployment for 0735258 failed]

@vercel vercel bot temporarily deployed to Preview February 7, 2022 16:40 Inactive
@github-actions github-actions bot added documentation Relates to documentation firebase @auth/firebase-adapter labels Feb 7, 2022
@vercel vercel bot temporarily deployed to Preview February 7, 2022 16:48 Inactive
@balazsorban44
Copy link
Member

balazsorban44 commented Feb 8, 2022

I really appreciate it! Once we finish the migration to a monorepo setup #3650, I'll have a look!

My main blocker on my original PR was to figure out how could we integrate with Firebase Authentication's User management.

Do you have any ideas? It would certainly be cleaner for Firebase users, but we could also just use Firestore for now and re-visit this later on.

I also know people wanted a security rule suggestion, so if you have any, could you add it to the docs at the same time?

@chanceaclark
Copy link
Contributor Author

chanceaclark commented Feb 8, 2022

I really appreciate it! Once we finish the migration to a monorepo setup #3650, I'll have a look!

For sure, no worries!

Do you have any ideas? It would certainly be cleaner for Firebase users, but we could also just use Firestore for now and re-visit this later on.

I was thinking about this for while and tried to take a stab at it myself adding it to this adapter. But I gave up after a little because it was like fitting a square peg in a round hole. Firebase Auth acts similarly to this library – providers in next-auth are similar in nature "Sign-in providers" in firebase/auth. Potentially we could make something work in y'alls Provider API but it might not be an all-in-one solution that we are looking for. For example I was thinking of something like FirebaseEmailProvider, FirebaseGoogleProvider, etc... I'm not too familiar with this library, but what do you think about that?

I also know people wanted a security rule suggestion, so if you have any, could you add it to the docs at the same time?

Do you know what kind of suggestion people are looking for? Like rules around the next-auth created collections (users, accounts, sessions and verificationTokens)? Or how to protect resources you create after a user has been authenticated? Also, is there a issue around this that I can get more context?

@balazsorban44 balazsorban44 added the adapters Changes related to the core code concerning database adapters label Feb 13, 2022
@vercel vercel bot temporarily deployed to Preview February 17, 2022 00:01 Inactive
@github-actions github-actions bot removed adapters Changes related to the core code concerning database adapters documentation Relates to documentation labels Feb 17, 2022
@alibidjand

This comment was marked as off-topic.

@chanceaclark

This comment was marked as off-topic.

@thiras
Copy link

thiras commented Mar 3, 2022

Hello. Is there any ETA for the merge?

@chanceaclark
Copy link
Contributor Author

Hey @thiras, I think we are still kinda waiting for #3952 to be resolved. Not sure what the status is on that, maybe @balazsorban44 or @ndom91 can enlighten us?

@balazsorban44
Copy link
Member

balazsorban44 commented Mar 7, 2022

The monorepo migration has been finished, currently making sure the release pipeline is ready. I'll look into PRs like this when I'm confident about releases. 👍

Thank you for your patience. In the meantime, please test this adapter by copy-pasting it into your projects and see if it works.

I see there is a merge conflict, that should be fixed anyway before this could be merged.

@chanceaclark
Copy link
Contributor Author

chanceaclark commented Mar 7, 2022

You got it @balazsorban44. I'll go ahead and rebase + test it. Also, it seems like one of the CI checks needs approval by a maintainer before running, would you mind checking that out?

EDIT: Updated some deps to the latest and tested locally - looks ✅

@vercel vercel bot temporarily deployed to Preview March 7, 2022 20:53 Inactive
@vercel vercel bot temporarily deployed to Preview March 7, 2022 20:57 Inactive
@vercel vercel bot temporarily deployed to Preview March 7, 2022 22:07 Inactive
@DiegoGonzalezCruz
Copy link

Hi guys, I just found this and I'm loving it! Sending y'all good energies for a successfull PR!

@MarinaMijatovic
Copy link

Can this adapter work with firebase realtime database?

@ndom91
Copy link
Member

ndom91 commented Mar 12, 2022

Can this adapter work with firebase realtime database?

This adapter is specifically designed to work with Firestore only. I'm curious what use-case you would need a realtime database for when storing user metadata (email, username, etc.)

@MarinaMijatovic
Copy link

Can this adapter work with firebase realtime database?

This adapter is specifically designed to work with Firestore only. I'm curious what use-case you would need a realtime database for when storing user metadata (email, username, etc.)

Sign in should be done by email provider, and the email provider requires adapter to store verification token. The project itself requires users metadata, and tracking current flow of their progress on their project. I'm not sure whether realtime database is the best solution, still researching.

@Sal-man
Copy link

Sal-man commented Mar 13, 2022

Hi guys, any ETA on the merge?

@ndom91
Copy link
Member

ndom91 commented Mar 14, 2022

Can this adapter work with firebase realtime database?

This adapter is specifically designed to work with Firestore only. I'm curious what use-case you would need a realtime database for when storing user metadata (email, username, etc.)

Sign in should be done by email provider, and the email provider requires adapter to store verification token. The project itself requires users metadata, and tracking current flow of their progress on their project. I'm not sure whether realtime database is the best solution, still researching.

Normal firestore -like database should be fine for this 👍

@chanceaclark chanceaclark requested a review from ndom91 June 29, 2022 15:59
@ndom91
Copy link
Member

ndom91 commented Jun 30, 2022

We fixed the docs failed deploy, should be good to go now.

It looks good to me, will try to get the teams support to merge this as soon as possible!

@vercel vercel bot temporarily deployed to Preview June 30, 2022 09:23 Inactive
@balazsorban44
Copy link
Member

balazsorban44 commented Jun 30, 2022

Thanks for the PR, and sorry for the delay! I'm thinking renaming to FirestoreAdapter is not a good idea, since the package is still called @next-auth/firebase-adapter. Won't that be confusing? 🤔

Do we see a future with a different Firebase-based adapter? Should we either:

  1. release @next-auth/firestore-adapter
  2. add this in a submodule @next-auth/firebase-adapter/firestore
  3. Let the export name be FirebaseAdapter

Apart from that, I had some code nitpicks, which I'll hold to myself for now, and can do in a follow-up PR, you have done great work here and I don't want to unnecessarily prolong the merge of this. 😅

@chanceaclark
Copy link
Contributor Author

Thanks for the PR, and sorry for the delay! I'm thinking renaming to FirestoreAdapter is not a good idea, since the package is still called @next-auth/firebase-adapter. Won't that be confusing? 🤔

I do agree with you that it is a little confusing, but I will say it's already confusing as is since this adapter is purely built for Firestore. We've seen the many comments of when Firebase Auth will be added to this adapter and we probably want to make it clear what this adapter actually supports: Firestore.

Do we see a future with a different Firebase-based adapter? Should we either:

  1. release @next-auth/firestore-adapter
  2. add this in a submodule @next-auth/firebase-adapter/firestore
  3. Let the export name be FirebaseAdapter

Unless I'm missing some context y'all might have, the only thing I potentially see is exporting a Firebase Realtime DB adapter. However, I've never worked with it so I don't know if it's even feasible/wanted. In regards to those options:

  1. I think that would be a lot of work and it's up to y'all if you want to go down that path of deprecating @next-auth/firebase-adapter – IMO I would leave it as is.
  2. Submodules is a fine approach, but you could also just export additional modules along with it if you are worried about namespacing them e.g. import { FirestoreAdapter, RealtimeAdapter } from '@next-auth/firebase-adapter 🤷
  3. I assume this is just revert the last commit just leave it as FirebaseAdapter.

My recommendation is the 2nd option I proposed, but at the end of the day it's your call. If you don't want to deal with the renaming quite yet, I'll rip it out, just give me the word. We can continue the discussion in #4592.

Apart from that, I had some code nitpicks, which I'll hold to myself for now, and can do in a follow-up PR, you have done great work here and I don't want to unnecessarily prolong the merge of this. 😅

I appreciate it! I also don't mind tackling them if need be, just let me know.

@cdriesler
Copy link

Since we're right at the finish line I want to jump in -

@chanceaclark do you have a sponsorship link somewhere, GitHub or otherwise? I've been eagerly following this PR for a while now. It'll solve a pile of problems for me, and I'd like to buy you a coffee-or-two as thanks for the months of work.

@chanceaclark
Copy link
Contributor Author

I appreciate the offer @cdriesler! However, I don't have anything setup for sponsorship currently and I'm doing this for the fun of it 😄 Maybe in the future, but for right now, just giving back to the community 🙇‍♂️

@vercel vercel bot temporarily deployed to Preview July 5, 2022 17:23 Inactive
@vercel vercel bot temporarily deployed to Preview July 7, 2022 15:38 Inactive
@vercel vercel bot temporarily deployed to Preview July 11, 2022 22:25 Inactive
BREAKING CHANGE:
Requires firebase v9+
BREAKING CHANGE:
Renames FirebaseAdapter export to FirestoreAdpater.
@balazsorban44 balazsorban44 merged commit 5bd00f6 into nextauthjs:main Jul 11, 2022
@chanceaclark chanceaclark deleted the feat/firebase-adapter branch July 12, 2022 02:37
@adrianvanlan
Copy link

When will this version be available in npm?

@balazsorban44
Copy link
Member

Good news, this is now available as @next-auth/firebase-adapter@latest! 🎉 Thanks to everyone who decided to stick around! 😅 👏

Let us know if you have any issues!

@dellwatson
Copy link

i just created new project and now have problem with firebaseAdapter:
name: 'GetUserByAccountError . it seems working now without next-auth, i suppose must be related to this?

@DanielBailey-web
Copy link

My main blocker on my original PR was to figure out how could we integrate with Firebase Authentication's User management.

Is this part of a new issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
firebase @auth/firebase-adapter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for the modular Firebase 9