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

Standardise FQL and returned objects. #1

Closed
wants to merge 3 commits into from
Closed

Standardise FQL and returned objects. #1

wants to merge 3 commits into from

Conversation

willllhallll
Copy link

Hey! Thanks so much for taking the Fauna adapter over to TypeScript and making some sweet improvements along the way. This PR hopefully just covers some further bases and deals with some sticking points I have been experiencing with the current adapter.

The major change is how dates are handled and returned. Currently, the response from Fauna contains and object with key value which holds an ISO time string, and is set directly in the return object. Now, value is used to construct and return a proper date object, as per the types. Null date objects are also directly handed now, rather than being set to undefined.

Next, I refactored some FQL to make it a little more semantic, and standardised the definition of theFQL as a const outside each try/catch block which is then used within a call to faunaClient.query. This allowed for the removal of the _deleteSession private function, for example, and gives a more readable and consistent style across adapter functions.

Another change is that most return object now contain the id of the object in question, derived from the Fauna ref, namely now including verification requests. This could be useful for performing actions with the verification request immediately after creation, or at least for debugging purposes alone.

Within the linkAccount function, the accounts object now no-longer returns any object and simply resolves the promise upon completion, as per the type requirements of the AdapterInstancer interface.

I also switched to the built-in next-auth logger, and added some creature comforts i.e. eslint & prettier. Feel free to remove these if they do not reflect the repo style.

Thanks for considering this PR. If I have missed anything or made any mistakes, please do let me know. I would be more than happy to revert or modify any changes that do not align with the general direction of the adapter. My aim was to improve the overall reliability of the adapter (especially returned objects) to ensure Fauna users have a 1st-class experience with next-auth.

Cheers!

P.s. Apologies that the diff is super messed up on the commits; I think the formatting and linting has properly mucked it up.

@n44ps
Copy link

n44ps commented Apr 22, 2021

Hey Will,

I have used your PR and worked on it !

  • Added the possibilities to set default values when creating User / Session / ..., important for custom models.
  • accessToken uses the Tokens from Fauna to enable ABAC and custom roles while trying to query over Fauna.
  • Updated the logic for Dates, using FQL which is for me easier to read than getTime(... + getTime())
  • Added ttl to delete Sessions directly from Fauna if expired, because it was still here even if expired.

Where do you think I should make those changes ? Should we talk about this here ?

@willllhallll
Copy link
Author

Hi @n44ps

That sounds super! I think the best place to talk about these would be on the original PR nextauthjs#43, since it appears that both mine and @johannes-scharlach's branch will need some work to move alongside the new typescript proposals.

It is a tricky situation since it is likely that there will be aspects needed to be rewritten from across all our branches, so I wouldn't want your work to be merged to a stale fork! Perhaps we should await the lead author's cue to move forward and co-ordinate ourselves then. Perhaps a new PR will be made, we shall see.

Comment on lines +1 to +7
module.exports = {
semi: true,
trailingComma: 'all',
singleQuote: true,
printWidth: 120,
tabWidth: 4,
};

Choose a reason for hiding this comment

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

I wonder if you'd be ok to go with

Suggested change
module.exports = {
semi: true,
trailingComma: 'all',
singleQuote: true,
printWidth: 120,
tabWidth: 4,
};
module.exports = {
semi: false,
trailingComma: 'es5',
singleQuote: true,
printWidth: 80,
tabWidth: 2,
};

This would keep your changes much closer to the original. The main reasons are

  1. the files used no semicolons so far, so the diff would be easier to review
  2. same for trailing commas (though this is minor and I don't mind either way)
  3. prettier strongly recommends a print width of 80, which I find very understandable
  4. changing the tab width also changed all the JSON files. And npm i will change the indentation of the package.json, which is annoying to format back and forth.

That would make it possible for me to actually take a look at the contents of your changes ;)

@johannes-scharlach
Copy link
Owner

Hi @WillTheVideoMan, I promise I didn't just ignore you ;) but I didn't want to put all the work on to you, but as it seems I don't really get to contribute to these changes as actively as I had hoped.

I'm also new to Fauna, so many of the FQL changes might not make so much sense to me, but I'll do my best to review. I would only prefer the more consistent formatting to help with reviewing.

@willllhallll
Copy link
Author

Hi @johannes-scharlach! Thanks for your review above, I will make sure to alter any future prettier configs to match your recommendation.

At this moment I have begun the process of rebasing my fork back to the original such that I can implement the new sweeping changes as discussed in nextauthjs#43. So for now I will close this PR and implement my own changes directly onto the base.

Of course, I will not implement your changes, as this would not properly attribute your time and energy towards the work. As such, after the PR is made, I would greatly appreciate if you might review it and re-introduce any of your changes / alterations as per this current fork.

Looking forward to your thoughts!

@johannes-scharlach
Copy link
Owner

@WillTheVideoMan awesome, I'm glad to hear that you're moving this forward.

I'd be happy if you'd also implement my changes if you feel like it. I had to make them in my own project and wanted to ensure that they are also contributed to the proper upstream repo. I don't care about the attribution. If you don't, I'll see if I find the time to contribute to it on top of your changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants