Skip to content
This repository has been archived by the owner on Jul 17, 2022. It is now read-only.

feat(firebase): use new Adapter API #183

Closed
wants to merge 5 commits into from

Conversation

balazsorban44
Copy link
Member

@balazsorban44 balazsorban44 commented Aug 16, 2021

See #171, #182

Firebase introduces a more modular version from 9 and up. The adapter has been rewritten to work with v8. I will see if I can make the upgrade:

https://firebase.google.com/docs/web/learn-more#v9-new-apps

A few things to figure out before we can merge this:

Any help with these would be appreciated! 🙏

@github-actions github-actions bot added fauna @next-auth/fauna-adapter related firebase @next-auth/firebase-adapter related tests Changes to testing labels Aug 16, 2021
@balazsorban44 balazsorban44 force-pushed the feat/refactor-firebase branch from e15fbb1 to 0e31ad2 Compare August 16, 2021 22:34
@github-actions github-actions bot removed the fauna @next-auth/fauna-adapter related label Aug 16, 2021
const newObjectobject: any = {}
for (const key in object) {
const value = object[key]
if (value === undefined) continue

Choose a reason for hiding this comment

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

👋 I've been playing around with this branch on a personal project and noticed a small issues when using the GoogleProvider for authentication. This line specifically checks for undefined, but in the case of a response from Google where emailVerified === null, reading the value below in fromFirestore on value.toDate will throw:

https://next-auth.js.org/errors#session_error Cannot read property 'toDate' of null TypeError: Cannot read property 'toDate' of null

Not sure what the best approach would be here but maybe you either want to:

  1. Exclude null values from when writing to firebase (probably not ideal and not recommended since null is a valid datatype) or...
  2. Ensuring value is defined below, before calling toDate(). I'll leave a suggestion below, or..
  3. All of above 🤷 Again, maybe not ideal since null is a valid datatype but you have more context than me 😅

const newUser: any = { ...snapshot.data(), id: snapshot.id }
for (const key in newUser) {
const value = newUser[key]
if (value.toDate) newUser[key] = value.toDate()

Choose a reason for hiding this comment

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

Suggested change
if (value.toDate) newUser[key] = value.toDate()
if (value && value.toDate) newUser[key] = value.toDate()

or even better

Suggested change
if (value.toDate) newUser[key] = value.toDate()
if (value?.toDate) newUser[key] = value.toDate()

Copy link

Choose a reason for hiding this comment

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

had the same issue with firebase v9. this fix works great.

Choose a reason for hiding this comment

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

Thank you for your suggestion! Awesome!

Copy link

Choose a reason for hiding this comment

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

yes, this solve the issue and it work in localhost. However, when i tried to deploy to Vercel, it could be build successfully, but it would have problem when login with google. When login with the google, it always show callback error, but in the localhost environmnet, there is no issue. is threre anything i should concern?

@amesas
Copy link

amesas commented Nov 9, 2021

Hi!

Is this pull request in a usable state?

* cleanup FirebaseClient

* fix undefined issue

* fix tests
@SahilAujla
Copy link

Does anyone know how to use @next-auth/firebase-adapter with NextAuth.js version 4?

@srijans38
Copy link

@balazsorban44 Please see https://github.com/nextauthjs/adapters/issues/180#issuecomment-1004126562. The issue has been closed by stale bot. I also have firebase auth working here #363. Need some feedback to see what more can be done.

@pawel-zaic
Copy link

@balazsorban44 is there any chance to upgrade firebase adapter in the near future?

@balazsorban44
Copy link
Member Author

This repository has been merged into a monorepo. Follow-up PR is here: nextauthjs/next-auth#3873

balazsorban44 pushed a commit to nextauthjs/next-auth that referenced this pull request Jul 11, 2022
Ports and refactors `@next-auth/firebase-adapter` to use the new Adapter API. Ported from this PR: nextauthjs/adapters#183

BREAKING CHANGE:

- Renames `FirebaseAdapter` export to `FirestoreAdpater`
- This adapter now requires firebase v9+
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
firebase @next-auth/firebase-adapter related help wanted Extra attention is needed tests Changes to testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants