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

Add fauna adapter #686

Closed
wants to merge 2 commits into from
Closed

Add fauna adapter #686

wants to merge 2 commits into from

Conversation

s-kris
Copy link

@s-kris s-kris commented Sep 18, 2020

@LoriKarikari

Adding faunadb adapter #682

@vercel vercel bot temporarily deployed to Preview September 18, 2020 04:44 Inactive
@vercel
Copy link

vercel bot commented Sep 18, 2020

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/6tjdie1pu
✅ Preview: https://next-auth-git-fork-s-kris-main.nextauthjs.vercel.app

@s-kris s-kris mentioned this pull request Sep 18, 2020
@iaincollins iaincollins added the enhancement New feature or request label Sep 18, 2020
@vercel vercel bot temporarily deployed to Preview September 19, 2020 14:13 Inactive
if (new Date() > dateSessionIsDueToBeUpdated) {
const newExpiryDate = new Date()
newExpiryDate.setTime(newExpiryDate.getTime() + sessionMaxAge)
session.expires = newExpiryDate

Choose a reason for hiding this comment

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

Shouldn't this be?
session.expires = newExpiryDate.valueOf()
If not fauna just stores an empty object (at least that's what I've been getting)

Copy link
Author

Choose a reason for hiding this comment

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

The logic is kept in sync with other adapters: https://github.com/nextauthjs/next-auth/tree/main/src/adapters
Let me check why fauna isn't saving this

? appOptions.session.maxAge * 1000
: defaultSessionMaxAge
const sessionUpdateAge =
appOptions && appOptions.session && appOptions.session.updateAge

Choose a reason for hiding this comment

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

Noticed this while testing with 0, was using the default.

appOptions && appOptions.session && (appOptions.session.updateAge || appOptions.session.updateAge === 0)

Copy link
Author

Choose a reason for hiding this comment

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

The logic is kept in sync with other adapters: https://github.com/nextauthjs/next-auth/tree/main/src/adapters
Can you raise an issue for this?

Choose a reason for hiding this comment

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

checked master, and both prisma and typeorm adapters have the right logic (fallback to 0).

Am I missing something here?

const sessionUpdateAge = (appOptions && appOptions.session && appOptions.session.updateAge)
      ? appOptions.session.updateAge * 1000
      : 0

async function createUser (profile) {
console.log('-----------createUser------------')
// console.log(profile);
return faunaWrapper(

Choose a reason for hiding this comment

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

I'm not sure if this is a pattern with fauna (new user as well) but to use the ref's across the other methods I basically return:

return await faunaClient.query(q.Create(q.Collection('users'), { data: profile }))
          .then(res => { return { ...res.data, id: res.ref } })

This way further down on other methods like getUser, I can directly use the id as follows:

    async function getUser (id) {
      try {
        return await faunaClient
          .query(q.Get(q.Ref(id)))
          .then(res => { return { ...res.data, id: res.ref } })
          .catch(err => Promise.resolve(null))
      } catch (error) {
        return Promise.reject(new Error('GET_USER_ERROR'), error)
      }
    }

Copy link
Author

Choose a reason for hiding this comment

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

It is a fauna pattern. It's optimal to use FQL way to execute multiple operations instead of using js patterns.

Choose a reason for hiding this comment

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

maybe it wasn't clear because of the line # of the comment, but I wasn't referring to the faunaWrapper. I was referring to the q.Select('data' part ... by returning the ref object as well, I was able to use the fauna IDs directly (Ref) rather than creating yet another set of id: fields for each object.

Which I'm not sure if fauna encourages that or encourages devs to handle their own IDs.

Copy link
Author

Choose a reason for hiding this comment

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

You are correct. We can directly use fauna ID but I just want to have id field with uuid so that I can simply extract the 'data' part of the document in case of migration to other db.

@ndom91
Copy link
Member

ndom91 commented Dec 6, 2020

Hi guys, tentative Fauna support was merged from PR #708 as that had tests and a few users who'd already used and vouched for the implementation. We still need some docs re: Fauna, and any other PRs improving the support or tests are always welcome as well!

@balazsorban44 balazsorban44 changed the base branch from main to canary December 6, 2020 20:49
@s-kris
Copy link
Author

s-kris commented Dec 8, 2020

@ndom91 thanks for the update, closing this.

@s-kris s-kris closed this Dec 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants