feat: set session to the context on a server side if available

[lnikell]

What:
The problem described in the #844 issue. Currently using getServerSideProps and getSession don't set session context properly which results in having an extra call on the client-side even if we get it already on the server-side.

Why:

• Avoiding extra network calls has a good impact on a performance
• It prevents confusion during development when you retrieve a session on the server-side but then have again a call on the client-side

How:

The issue comes from the Provider, where context was always set by fetching the context with useSession. In the useSession however, there is a fallback to do a useSessionHook. This means even if we already have Session, we anyway set it only from the client-side.

next-auth/src/client/index.js

Lines 294 to 297 in 416d92c

	export const Provider = ({ children, session, options }) => {
	setOptions(options)
	return createElement(SessionContext.Provider, { value: useSession(session) }, children)
	}

The fix is simple, just set the value to the context which is an array of session + loading if we already have it on the server side.

https://github.com/lnikell/next-auth/blob/1c7be413f56ee304b129c31763053a4597f8a131/src/client/index.js#L294-L298

Before:

Screen.Recording.2021-01-11.at.12.39.49.mov

After:

Screen.Recording.2021-01-11.at.12.37.51.mov

Checklist:

• Documentation n/a
• Tests n/a
• Ready to be merged

[vercel]

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/aimedv6t5
✅ Preview: https://next-auth-git-fork-lnikell-canary.nextauthjs.vercel.app

[balazsorban44]

I really appreciate the before/after videos on your contributions, very helpful! Also, thanks for digging into this problem.

I am not sure if this is the perfect solution though, as it breaks the Rules of Hooks. (It does so already, _useSessionHook is not exactly called right either). I am wondering if we could work this around another way.

[lnikell]

@balazsorban44 thanks, good point about hooks rules, I will refactor it a little bit.

[iaincollins]

@lnikell This is great work, thank you! @balazsorban44 good catch consideration for Rules of Hooks

I think I tried to refactor this once but gave up because I couldn't work out how to do it "right". :-/

FWIW if we can't figure out how to do it without breaking the rules but we don't see any side effects in practice, personally I have no objection to going ahead anyway (although I agree, doing it without breaking them is the best scenario!).

[balazsorban44]

I had a go at this at #943 but I wasn't satisfied, and got unsure if I broke anything so did not proceed. I might get back to it at a later point, but I would like to see if we come up with something smart on this PR.

[lnikell]

@balazsorban44 @iaincollins I spent some time learning more the code that related to this issue and I think we can consider removing the "useSession" execution from the `Provider.

My assumptions:

• Provider should only keep session data and don't make an api call and also keep a loading state.
• Having useSession inside Provider creates an extra "magic" logic and can confuse users(such as me). I was confused the first time when I get calls to the "session" endpoint without having anywhere in my code useSession method.
• If we remove useSession call from Provider we will have to add extra state to Provider to set the session, which we will run from useSession in order to update the context.

So as a temprorary solution we can go with what we have in this PR, and later consider refactoring of the Provider. I can try to make those changes if you confirm.

[kyds3k]

I think this is causing me MANY problems! Is there any update on getting it into the main release? What can I do in the meantime?

[iaincollins]

I think this is causing me MANY problems! Is there any update on getting it into the main release? What can I do in the meantime?

I would suspect it's not the cause of any problems you are having as it's more an optimisation.

Can you elaborate on sort of problems are you having and how do you think this would resolve them?

PS: If you haven't already, it might be best to post in Discussions about them and link back here.

[rozenmd]

FWIW: I did a few Lighthouse tests to compare before/after of the workaround described here - the extra getSession request can increase speed index by up to 3.0 seconds.

Pretty worthwhile optimisation if you ask me.

[balazsorban44]

So I think I managed to work around this, without breaking the Rules of Hooks! Added in this commit: 231a66c

We can do the state initialization smarter. The interesting part is:

const [data, setData] = useState(context?.[0] ?? session)
const [loading, setLoading] = useState(!data)
...
useEffect(

So instead of calling the useEffect optionally, we set the state by first checking the context value.

One potential downside for this is that the useEffect has to be called on each render (notice in the commit, there is no dependency array at the end), but early bailout ensures that we still only fetch the session when it is needed, as before. If we wanted to go even further with this, we should move the listener logic inside the Provider, but that would require useSession() to always have a Provider above it for all the current features to work, which is a breaking change.

My solution at least prevents the extra fetch if you already provided a session which I think people here wanted. We could further optimize in a later release.

DISCLAIMER: My PR on this (#1428) is still a draft, and I have to discuss it first with Iain. So don't expect anything just yet. I just wanted to show my take on this.

[balazsorban44]

Closing in favor of #1428. Thank you so much for your examples @lnikell, they helped a lot while evaluating this!