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

Reuse previous session in SessionProvider #4252

Closed
wants to merge 1 commit into from

Conversation

skirsten
Copy link
Contributor

Reasoning 💡

This reuses the previous session from the last mounted <SessionProvider>, if present.
With this change, users don't have to add the <SessionProvider> to the _app.js but can instead add it to individual pages that require authentication.

This reduces bundle size for pages that do not require authentication (e.g. landing pages, documentation) and does not create cookies until the user enters a page that uses the provider.

Technically this was possible before, but if the user went from a non-authenticated page back to a authenticated page, the session would not be loaded.

Checklist 🧢

  • Documentation
  • Tests
  • Ready to be merged

Affected issues 🎟

@vercel
Copy link

vercel bot commented Mar 25, 2022

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/J9si6EAjkZHiUcmMZFk1Scn5JzdF
✅ Preview: Canceled

[Deployment for 43245a6 canceled]

@github-actions github-actions bot added client Client related code core Refers to `@auth/core` labels Mar 25, 2022
Copy link
Member

@balazsorban44 balazsorban44 left a comment

Choose a reason for hiding this comment

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

I think this should also update the hasInitialSession value with && __NEXTAUTH._session !== undefined to take this previous value's existence into consideration:

https://github.com/nextauthjs/next-auth/blob/d4568c5a7d78f86a7869302ecd0ce68371c43c68/packages/next-auth/src/react/index.tsx#L306

Otherwise, an existing session might be reset if props.session was null, and the loading state would start with true for that context, even though there is a session already.

I am not entirely sure of the gains with multiple SessionProvider though. 🤔 Could you explain? Right now it just sounds like a bit of overcomplicating things.

@skirsten
Copy link
Contributor Author

skirsten commented Mar 29, 2022

Yes, you are right. I updated the commit so the "props session" has the highest priority and then the old client-side session.

I am not entirely sure of the gains with multiple SessionProvider though. thinking Could you explain? Right now it just sounds like a bit of overcomplicating things.

The idea is not to use multiple SessionProvider but to instead only use the SessionProvider on some pages.
For example in my _app I did not add the SessionProvider.

So the landing / and /docs/* etc. layouts do not have the SessionProvider which makes the bundle (slightly) smaller and does not create any cookies when somebody is just browsing the docs.

But in the /console/* where I want to use the authentication, I include the SessionProvider on the layout.

The problem this PR fixes is, if somebody navigates from /console to /docs/xyz and back to /console, which was not working before (on Next.js-like apps with client-side routing).

Also, I use puppeteer to generate screenshots of pages for preview images. And I don't want this headless worker to generate a bunch of session requests.

@skirsten
Copy link
Contributor Author

Closing this in favor of #4383.
This PR had some problems with multiple tabs in which some were mounted and some unmounted components.
#4383 does not reuse the session but instead just fixes the bug that caused me to create this PR initially.

@skirsten skirsten closed this Apr 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client Client related code core Refers to `@auth/core`
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants