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.

Sign up for GitHub

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

Jump to bottom

feat: ✨ add flag to disable getSession after signIn on local / refresh provider #702

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

feat: ✨ add flag to disable getSession after signIn on local / refresh provider #702

wants to merge 10 commits into from
+16 −3

Conversation

bitfactory-frank-spee
Copy link

@bitfactory-frank-spee bitfactory-frank-spee commented Mar 5, 2024 •
edited
Loading

🔗 Linked issue

#701

❓ Type of change

  • 📖 Documentation (updates to the documentation, readme or JSdoc annotations)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • 👌 Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • 🧹 Chore (updates to the build process or auxiliary tools and libraries)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

📚 Description

See issue for clarification. This adds a flag to the signInOptions named withSession which defaults to true. This makes sure this will be backwards compatible. When withSession === false the getSession method will not be called.

This also includes a bug fix (681fdb0) where refresh was not equal to local provider.

📝 Checklist

  • I have linked an issue or discussion.
  • I have added tests (if possible).
  • I have updated the documentation accordingly.

NOTE: this documentation mentioned withGetSession before and not withSession as decided to be the name later.

@bitfactory-frank-spee bitfactory-frank-spee mentioned this pull request Mar 5, 2024
Open
5 tasks
@phoenix-ru
Copy link
Collaborator

Hi @bitfactory-frank-spee , thanks for your contribution, it looks good to me.

I will come back to it after #694 , so that we can ensure that it's functionally good.

@zoey-kaiser zoey-kaiser linked an issue Mar 7, 2024 that may be closed by this pull request
feat: ✨ add flag to disable getSession after signIn on local / refresh provider #701
Open
5 tasks
@phoenix-ru
Copy link
Collaborator

@bitfactory-frank-spee Could you please sync your PR with the current main?

@phoenix-ru
Copy link
Collaborator

bump @bitfactory-frank-spee

@bitfactory-frank-spee
Copy link
Author

@phoenix-ru I will update it this week 👍

@bitfactory-frank-spee
Copy link
Author

@bitfactory-frank-spee Could you please sync your PR with the current main?

Done 🚀

@zoey-kaiser zoey-kaiser added enhancement An improvement that needs to be added provider-local An issue with the local provider provider-refresh An issue with the refresh provider labels May 9, 2024
zoey-kaiser
zoey-kaiser requested changes May 22, 2024
View reviewed changes
Copy link
Member

@zoey-kaiser zoey-kaiser left a comment

Choose a reason for hiding this comment

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

Hi @bitfactory-frank-spee 👋

Very close to being done! I have one minor renaming request and then we should be good to go, to release this in 0.8!

docs/content/3.application-side/2.session-access-and-management.md Outdated Show resolved Hide resolved
@zoey-kaiser
Copy link
Member

Hi @bitfactory-frank-spee 👋

We just released 0.8.0, so I would begin looking into getting this PR merged in the next release. Could you please merge main and we can then do a final round of reviews 😊

zoey-kaiser
zoey-kaiser requested changes Jul 5, 2024
View reviewed changes
Copy link
Member

@zoey-kaiser zoey-kaiser left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Just some typings missing 😊

src/runtime/composables/local/useAuth.ts
await nextTick(getSession)
const { callbackUrl, redirect = true, external, withSession = true } = signInOptions ?? {}

if (withSession) {
Copy link
Member

Choose a reason for hiding this comment

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

Could you please add type support for the new withSession option? Please also consider, that you will need to account for the fact that this value is only available for the local and refresh providers and should therefore not be added to the signIn function of the authjs provider.

Copy link
Author

Choose a reason for hiding this comment

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

Hi Zoey. Sorry it took a while. In my mind I responded somewhere. I can add typing.

But can you direct / help me to how I do this "account for the fact that this value is only available for the local and refresh providers and should therefore not be added to the signIn function of the authjs provider"? Because I have no idea what you mean here.

Copy link
Author

Choose a reason for hiding this comment

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

So where do I add the typing support?

Copy link
Author

Choose a reason for hiding this comment

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

And how do I make sure this is not added to the signIn function of the authjs provider?

Copy link
Author

Choose a reason for hiding this comment

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

Is this enough? https://github.com/sidebase/nuxt-auth/pull/702/files#diff-23ace85c1398da87fad352ed08fd7026dab0a96402ccc8b6c3e23b478c993f43

Copy link
Author

Choose a reason for hiding this comment

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

@zoey-kaiser ?

@zoey-kaiser
Copy link
Member

Closing as this PR has gone stale. If someone else would like to take over the PR, feel free to fork the fixes and open a new PR 😊

@Mucaccino
Copy link

I need this. Really this was gone stale?

@bitfactory-frank-spee
Copy link
Author

I need this. Really this was gone stale?

Ahh, I now see that I needed to respond to this, and that I didn't. I have now: #702 (comment)

@zoey-kaiser zoey-kaiser reopened this Oct 22, 2024
@bitfactory-frank-spee
Copy link
Author

I merged the current main into this PR and fixed the conflicts as the local and refresh provider are merged in the new release. I also fixed the documentation.

bitfactory-frank-spee
bitfactory-frank-spee commented Nov 5, 2024
View reviewed changes
src/runtime/types.ts
*
* @default true
*/
withSession?: boolean
}

export interface SignUpOptions extends SecondarySignInOptions {
Copy link
Author

Choose a reason for hiding this comment

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

This interface is odd IMHO. Because I never see this used in the code with any of the properties of the SecondarySignInOptions interface. It is just used next to a property which uses SecondarySignInOptions. But I could miss something which is there somewhere.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zoey-kaiser zoey-kaiser zoey-kaiser requested changes

@phoenix-ru phoenix-ru Awaiting requested review from phoenix-ru

@bitfactory-oussama-fadlaoui bitfactory-oussama-fadlaoui Awaiting requested review from bitfactory-oussama-fadlaoui

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
enhancement An improvement that needs to be added provider-local An issue with the local provider provider-refresh An issue with the refresh provider
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

feat: ✨ add flag to disable getSession after signIn on local / refresh provider
5 participants
@bitfactory-frank-spee @phoenix-ru @zoey-kaiser @Mucaccino @bitfactory-oussama-fadlaoui