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

Merged
merged 15 commits into from
Dec 19, 2024
Merged

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

merged 15 commits into from
Dec 19, 2024

Conversation

bitfactory-frank-spee
Copy link
Contributor

@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
Closed
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
Closed
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
Contributor Author

@phoenix-ru I will update it this week 👍

@bitfactory-frank-spee
Copy link
Contributor Author

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

Done 🚀

@zoey-kaiser zoey-kaiser requested a review from phoenix-ru May 9, 2024 10:58
@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 previously 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 Outdated Show resolved Hide resolved
@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
Contributor 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
Contributor 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.

@pkg-pr-new pkg.pr.new
Copy link

pkg-pr-new bot commented Nov 28, 2024 •
edited
Loading

Open in Stackblitz

npm i https://pkg.pr.new/@sidebase/nuxt-auth@702

commit: 32f3e80

phoenix-ru
phoenix-ru reviewed Nov 28, 2024
View reviewed changes
Copy link
Collaborator

@phoenix-ru phoenix-ru 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 , sorry for us not answering for a long time, I would take over the PR and make sure it's over the line :)

docs/guide/application-side/session-access.md Outdated Show resolved Hide resolved
@phoenix-ru
Copy link
Collaborator

phoenix-ru commented Nov 28, 2024 •
edited
Loading

@bitfactory-frank-spee, unfortunately, I cannot directly commit to your PR (probably due to restrictions on your org).

I still checked it though and it seems to work, but I have several points

@phoenix-ru phoenix-ru mentioned this pull request Nov 28, 2024
Open
phoenix-ru
phoenix-ru reviewed Nov 28, 2024
View reviewed changes
src/runtime/types.ts Outdated Show resolved Hide resolved
src/runtime/types.ts Outdated Show resolved Hide resolved
@bitfactory-frank-spee
Copy link
Contributor Author

Hi @bitfactory-frank-spee , sorry for us not answering for a long time, I would take over the PR and make sure it's over the line :)

No problem. I will check your comments today.

@bitfactory-frank-spee
Copy link
Contributor Author

@bitfactory-frank-spee, unfortunately, I cannot directly commit to your PR (probably due to restrictions on your org).

I still checked it though and it seems to work, but I have several points

Ah that could be. I am sorry about that. I can probably not change that immediately. Next time I will use my personal account.

@bitfactory-frank-spee
Copy link
Contributor Author

@phoenix-ru I fixed everything you asked. And updated this branch with the main branch.

@phoenix-ru
Copy link
Collaborator

Hi @bitfactory-frank-spee , extremely sorry for deprioritizing this for a long time! I will ask @zoey-kaiser if she's fine with squeezing this PR into the upcoming 0.10.0 release and merge asap

@zoey-kaiser zoey-kaiser merged commit b5af548 into sidebase:main Dec 19, 2024
6 checks passed
@bitfactory-frank-spee
Copy link
Contributor Author

Hi @bitfactory-frank-spee , extremely sorry for deprioritizing this for a long time! I will ask @zoey-kaiser if she's fine with squeezing this PR into the upcoming 0.10.0 release and merge asap

Excellent! 🎉 🚀 Thanks for the help. Good luck with further releases.

@bitfactory-frank-spee bitfactory-frank-spee deleted the feature/optional-get-session-on-sign-in branch December 22, 2024 17:43
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 approved these changes

@phoenix-ru phoenix-ru phoenix-ru approved these changes

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

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