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

SSR + useSession causes an extra network call and render cycle #844

Closed
2 of 5 tasks
joeltg opened this issue Nov 11, 2020 · 17 comments
Closed
2 of 5 tasks

SSR + useSession causes an extra network call and render cycle #844

joeltg opened this issue Nov 11, 2020 · 17 comments
Labels
bug Something isn't working stale Did not receive any activity for 60 days

Comments

@joeltg
Copy link

joeltg commented Nov 11, 2020

Describe the bug

If you use getSession to return a session prop from getServerSideProps and then use useSession in the component, the useSession hook will cause the page to re-render twice (three times total) even though it returns the valid session object all three times.

In addition, the hook will still cause the client to make a network request to the server after its first render, even though it already has the session object.

Steps to reproduce

You can observe this behavior by adding a console log to the render method of /server in next-auth-example and watching network requests that it makes.

Expected behavior

As far as I can tell, the expected behavior would go like this for a server-side rendered page:

  1. the incoming request causes nextjs to to call getServerSideProps, which calls getSession and returns the session in pageProps.session which is passed into the context provider in _app.tsx
  2. nextjs renders the page on the server, where useSession returns [session, false] since it is able to retrieve the session object from the Provider context
  3. the client hydrates and renders for the first time, and useSession again returns [session, false] since it is able to retrieve the session object from the Provider context
  4. no additional renders or network requests are triggered

I could definitely be mistaken about what's expected - but my understanding from the Provider documentation is that there should not be an extra network request.

Screenshots or error logs

What instead happens is that the client renders three times:

  1. on the first render, useSession returns [session, true] (where session is the full valid session object)
  2. the client dispatches a network request to /session
  3. the client re-renders, useSession returns [session, true] (the same as the first render)
  4. the network request resolves
  5. the client re-renders again, useSession returns [session, false]

E16F1738-F7C5-4250-8BEA-31C85B6EFB70-658-000107070BB4C866

AAD5D18A-7144-4A4F-B89C-DD71D87F0208-658-00010708B7239CDB

As another example, if you block network requests to /session, then the page first renders a logged-in view (once), and then a logged-out view (twice):

06364797-9E09-4CF4-955C-B195C73806CC-658-00010753FBBEAA16

These were tested on both development and production builds, without un-focusing or re-focusing the window. I can't think of anything else that could be causing the re-renders.

Additional context

I think that #487 originally encountered the same root problem, although their complaint was about the session callback (on the sever) getting triggered several times and the discussion ended up addressing ways to avoid using the session callback altogether.

Adding a session callback with a console log to the example will show that it is invoked twice on every /server page request - once during SSR and then again when the client makes its network request after rendering once.

Feedback

  • Found the documentation helpful
  • Found documentation but was incomplete
  • Could not find relevant documentation
  • Found the example project helpful
  • Did not find the example project helpful

Thanks so much for all your work on this project! Overall it's been a joy to use.

@joeltg joeltg added the bug Something isn't working label Nov 11, 2020
@joeltg joeltg changed the title SSR + useSession still results in an extra network call and render cycle SSR + useSession causes an extra network call and render cycle Nov 11, 2020
@aryascripts
Copy link

Did you ever find the fix for this or does it absolutely require a fix on next-auth side?

@joeltg
Copy link
Author

joeltg commented Nov 25, 2020

I think doing it "right" does require some kind of fix or re-structuring on the next-auth side, but I can share a workaround I've been using in the meantime.

The basic goal is to write our own context that provides the session object.

The naive approach looks like this in _app.tsx:

import { getSession, Session } from "next-auth/client";

type ExpandedAppProps = AppProps & { session: Session };

const App = ({ Component, pageProps, session }: ExpandedAppProps) => {
	return (
		<MyCustomContext.Provider value={{ session }}>
			<Component {...pageProps} />
		</MyCustomContext.Provider>
	);
};

App.getInitialProps = async ({ ctx }: AppContext) => {
	const session = await getSession(ctx);
	return { session };
};

The problem with this is that there's no way to access the session from getServerSideProps, since getServerSideProps is only passed a ctx: GetServerSidePropsContext object. I don't think this ctx is always the same (===) one that is passed to App.getInitialProps, but the request object ctx.req definitely is the same for both.

What does this mean? It means this is a perfect place to use a WeakMap!

In some common file utils.ts:

import { Session, getSession } from "next-auth/client";

const sessionCache = new WeakMap<NextApiRequest, Session | null>()

export async function getCachedSession({ req }: { req: NextApiRequest }): Session | null {
  const session = sessionCache.get(req)
  if (sessionCache.has(req)) {
    return sessionCache.get(req)!
  } else {
    const session = await getSession({ req })
    sessionCache.set(req, session)
    return session
  }
}

then you can call await getCachedSession(ctx) from both App.getInitialProps and from getServerSideProps and it'll only end up calling getSession once. Then on the client, we can get the session with useContext(MyCustomContext) whenever we need it.

Hope this helps! It's pretty hacky - it'd be really great if the next-auth provider could encapsulate this.

(you might have to mess around with the types a little bit since I think ctx.req is a NextApiRequest when it's in App.getInitialProps but just a plain IncomingMessage when it's in getServerSideProps or something...)

@balazsorban44
Copy link
Member

balazsorban44 commented Dec 9, 2020

Hi there! A reproduction repo with a link would be extremely helpful to debug your issue. 🙂

In your getServerSideProps, you should return the session in props.
Please see the Next.js docs.

Then in your _app, you should pass pageProps.session to the Provider ({ Provider } from 'next-auth/client')

If you could verify the behavior you are describing with a minimal reproduction, then maybe it is something in next-auth.

UPDATE: sorry, I have to read through the OP's comment a bit more thoroughly, you say it happens with our official example as well. I'll have a look.

@balazsorban44 balazsorban44 added incomplete Insufficient reproduction. Without more info, we won't take further actions/provide help. and removed incomplete Insufficient reproduction. Without more info, we won't take further actions/provide help. labels Dec 9, 2020
@balazsorban44
Copy link
Member

balazsorban44 commented Dec 9, 2020

So I found something, which might actually be a bug on our end:

The useEffect hook has no dependency array, which makes it effectively worthless, and could be the reason that the /session API is called several times.

Also, there is something weird in the useSession implementation. It takes a session parameter, but it is not even documented.

const useSession = (session) => {

So this might be some kind of a leftover, I think.

To check out the useEffect issue, would you @joeltg be comfortable with trying to go to your node_modules/next-auth/dist/client.js, and add an empty [] to the useEffect that needs it, and see if you get different results?

@joeltg
Copy link
Author

joeltg commented Dec 13, 2020

The file node_modules/next-auth/dist/client.js is minified or something, so I'm not 100% sure that I'm doing it right, but as far as I can tell, adding the empty dependency array doesn't change anything in the behavior of the server-side-rendered page.

@balazsorban44
Copy link
Member

thanks, I'll keep digging then

lnikell added a commit to lnikell/next-auth that referenced this issue Jan 11, 2021
@stale stale bot added the stale Did not receive any activity for 60 days label Feb 11, 2021
@nextauthjs nextauthjs deleted a comment from stale bot Feb 11, 2021
@stale
Copy link

stale bot commented Feb 18, 2021

Hi there! It looks like this issue hasn't had any activity for a while. To keep things tidy, I am going to close this issue for now. If you think your issue is still relevant, just leave a comment and I will reopen it. (Read more at #912) Thanks!

@stale stale bot closed this as completed Feb 18, 2021
balazsorban44 added a commit that referenced this issue Mar 8, 2021
These changes ensure that we work more tightly with React
that can also result in unforeseen performance boosts.
In case we would decide on expanding
to other libraries/frameworks, a new file per framework could be added.
We might also want to update the import
from `next-auth/client` to `next-auth/react`.

BREAKING CHANGE: Some performance issues (#844)
could only be fixed by moving setting up event listeners
inside `Provider`. This breaks the current tab/window sync behavior
for users who don't use `Provider` in their app. For these users,
wrap your app in `<Provider>` at a level above all of  your `useSession` calls.
If you don't care about tab/window syncing, this might not be needed,
but still recommended.
balazsorban44 added a commit that referenced this issue Jun 11, 2021
**What**:

These changes ensure that we work more tightly with React that can also result in unforeseen performance boosts. In case we would decide on expanding to other libraries/frameworks, a new file per framework could be added.

**Why**:

Some performance issues (#844) could only be fixed by moving more of the client code into the `Provider`.

**How**:

Refactoring `next-auth/client`

Related: #1461, #1084, #1462

BREAKING CHANGE:
**1.** `next-auth/client` is renamed to `next-auth/react`.

**2.** In the past, we exposed most of the functions with different names for convenience. To simplify our source code, the new React specific client code exports only the following functions, listed with the necessary changes:

- `setOptions`: Not exposed anymore, use `SessionProvider` props
- `options`: Not exposed anymore, use `SessionProvider` props
- `session`: Rename to `getSession`
- `providers`: Rename to `getProviders`
- `csrfToken`: Rename to `getCsrfToken`
- `signin`: Rename to `signIn`
- `signout`: Rename to `signOut`
- `Provider`: Rename to `SessionProvider`

**3.** `Provider` changes.
- `Provider` is renamed to `SessionProvider`
- The `options` prop is now flattened as the props of `SessionProvider`.
- `clientMaxAge` has been renamed to `staleTime`.
- `keepAlive` has been renamed to `refetchInterval`.
An example of the changes:
```diff
- <Provider options={{clientMaxAge: 0, keepAlive: 0}}>{children}</Provider>
+ <SessionProvider staleTime={0} refetchInterval={0}>{children}</SessionProvider> 
```

**4.** It is now **required** to wrap the part of your application that uses `useSession` into a `SessionProvider`.

Usually, the best place for this is in your `pages/_app.jsx` file:

```jsx
import { SessionProvider } from "next-auth/react"

export default function App({
  Component,
  pageProps: { session, ...pageProps }
}) {
  return (
    // `session` comes from `getServerSideProps` or `getInitialProps`.
    // Avoids flickering/session loading on first load.
    <SessionProvider session={session}>
      <Component {...pageProps} />
    </SessionProvider>
  )
}
```
@3li7u
Copy link

3li7u commented Dec 9, 2021

Hi there!
I am writing this comment while I am using version 4, in fact, I am not entirely sure if my problem is related to what was presented in this problem or that the fixes that were introduced solve my problem, but despite all this, I am facing a similar problem, I will describe it.
I use the useSession hook in order to get the session and store it in context, it works fine except it sends many extra requests to the /api/auth/session which are useless after actually getting the session for the first time, I mean the session is checked repeatedly whenever lose focus and refocus the window.
I attached a screenshot of the successive requests.

Screenshot (113)

I am looking forward to getting rid of these requests, I hope this is clear.

@ivan-kleshnin
Copy link

ivan-kleshnin commented Jun 23, 2022

@3li7u it's a classic case of "The road to hell is paved with good intentions". React and Next-Auth try to help us a bit too much...

  1. Disable strict mode of React.
  2. useSession does not deduplicate queries. Don't use SessionProvider and useSession and write your own useSession hook like https://github.com/nextauthjs/react-query/blob/main/index.js
    It can be based on React-Query or Apollo-Client or URQL or whatever you prefer.

👉 You really want to fetch & cache all data with a single "state-management-and-fetcher" tool (of your choice):

  • to rely on a single devtools and see all data there!
  • to rely on a single cross-tab sync tool!
  • to not rehydrate in 2 different ways after SSR!

To me, using an out-of-the-box useSession feels like using an out-of-the-box signin form.
It's approapriate as a one-off, dev-mode solution but it's not intended to be a final code.


I especially question the design decision to have a SessionProvider that triggers the first fetch.
In my mind no component named like *Provider* should ever fetch or do other effects.

@ivan-kleshnin
Copy link

ivan-kleshnin commented Jun 23, 2022

@balazsorban44 would be really helpful to see your opinion on the above.

Next-Auth is an amazing project but I wish it positioned itself as primarily a BE library, with maybe few low-level FE helpers (while it stays framework agnostic). Do you agree that higher-level FE tools, like the above useSession not only bloat the codebase but also immediately clash and compete with other tools in the ecosystem?

20 lines of code in the docs (trivial to readapt for any fetching/rendering soution) could replace next-auth/react, next-auth/angular and other numerous FE "adapters" you now have to support 😨

@balazsorban44
Copy link
Member

balazsorban44 commented Jun 23, 2022

We are working on making the next-auth core fully framework agnostic. 👍

you'll be able to implement your own client however you want, or just use the REST api directly (that you can already do, see https://next-auth.js.org/getting-started/rest-api)

@GermanJablo
Copy link

Does the fact that this thread is closed mean that there are no plans to fix the provider or hook? Could it be opened again?

@thilllon
Copy link

thilllon commented Jan 13, 2023

@3li7u it's a classic case of "The road to hell is paved with good intentions". React and Next-Auth try to help us a bit too much...

  1. Disable strict mode of React.
  2. useSession does not deduplicate queries. Don't use SessionProvider and useSession and write your own useSession hook like https://github.com/nextauthjs/react-query/blob/main/index.js
    It can be based on React-Query or Apollo-Client or URQL or whatever you prefer.

👉 You really want to fetch & cache all data with a single "state-management-and-fetcher" tool (of your choice):

  • to rely on a single devtools and see all data there!
  • to rely on a single cross-tab sync tool!
  • to not rehydrate in 2 different ways after SSR!

To me, using an out-of-the-box useSession feels like using an out-of-the-box signin form. It's approapriate as a one-off, dev-mode solution but it's not intended to be a final code.

I especially question the design decision to have a SessionProvider that triggers the first fetch. In my mind no component named like *Provider* should ever fetch or do other effects.

Thanks, @ivan-kleshnin
However, for what reasons should we Disable strict mode of React. ? https://reactjs.org/docs/strict-mode.html

@ilia-os
Copy link

ilia-os commented Jan 19, 2023

@thilllon

Thanks, @ivan-kleshnin However, for what reasons should we Disable strict mode of React. ? https://reactjs.org/docs/strict-mode.html

The reason is React 18 with strict mode enabled will render components twice in dev environment.
https://beta.reactjs.org/learn/keeping-components-pure#detecting-impure-calculations-with-strict-mode

@ivan-kleshnin
Copy link

ivan-kleshnin commented Feb 1, 2023

The reason is React 18 with strict mode enabled will render components twice in dev environment.
https://beta.reactjs.org/learn/keeping-components-pure#detecting-impure-calculations-with-strict-mode

Which doesn't help all that much in practice but surely wastes a lot of CPU ticks, reducing your laptop battery life 😨
As an engineer, I would be ashamed of an idea to consequently render 2x times only to expose architectural defects that shouldn't have been possible in the first place. It's "dev mode" only, yes, but it's still ridiculous.

@gavinthomas-valtech
Copy link

useSession does not deduplicate queries. Don't use SessionProvider and useSession

is this still the case?

Does it not cache the session information rather than calling /api/session each time?

@gautamsarawagi
Copy link

gautamsarawagi commented Jul 20, 2024

@balazsorban44 @ivan-kleshnin I was still experincing the issue. And this got resolved by creating a custom useMemo hook.

It would be great to get your input on this,

"use client";

import "@stream-io/video-react-sdk/dist/css/styles.css";
import { Room } from "../../../db/schema";
import {
  Call,
  CallControls,
  SpeakerLayout,
  StreamCall,
  StreamTheme,
  StreamVideo,
  StreamVideoClient,
} from "@stream-io/video-react-sdk";
import { useSession } from "next-auth/react";
import { useEffect, useMemo, useState } from "react";
import { generateTokenAction } from "./actions";
import { useRouter } from "next/navigation";

const apiKey = process.env.NEXT_PUBLIC_GET_STREAM_API_KEY!;

function useStableSession(){
  const {data:sessionData,status} = useSession()

  return useMemo(() => {
   return {
    session : sessionData,
    status,
    isAuthenticated: status === "authenticated"
   }
  },[sessionData?.user?.id, status])
}

export function DevFinderVideo({ room }: { room: Room }) {
  
  const {session} = useStableSession()

  const [client, setClient] = useState<StreamVideoClient | null>(null);
  const [call, setCall] = useState<Call | null>(null);

  const router = useRouter();

  useEffect(() => {
    if (!room) return;
    if (!session) {
      return;
    }
    const userId = session.user.id;
    const client = new StreamVideoClient({  
      apiKey,
      user: {
        id: userId,
        name: session.user.name ?? undefined,
        image: session.user.image ?? undefined,
      },
      tokenProvider: () => generateTokenAction(),
    });
    const call = client.call("default", room.id);
    call.join({ create: true });
    setClient(client);
    setCall(call);

    return () => {
      call
        .leave()
        .then(() => client.disconnectUser())
        .catch(console.error);
    };
  }, [session, room]);

  return (
    client &&
    call && (
      <StreamVideo client={client}>
        <StreamTheme>
          <StreamCall call={call}>
            <SpeakerLayout />
            <CallControls 
            onLeave={() => {
              router.push('/')
            }}
            />
          </StreamCall>

        </StreamTheme>
      </StreamVideo>
    )
  );
}

mnphpexpert added a commit to mnphpexpert/next-auth that referenced this issue Sep 2, 2024
**What**:

These changes ensure that we work more tightly with React that can also result in unforeseen performance boosts. In case we would decide on expanding to other libraries/frameworks, a new file per framework could be added.

**Why**:

Some performance issues (nextauthjs#844) could only be fixed by moving more of the client code into the `Provider`.

**How**:

Refactoring `next-auth/client`

Related: nextauthjs#1461, nextauthjs#1084, nextauthjs#1462

BREAKING CHANGE:
**1.** `next-auth/client` is renamed to `next-auth/react`.

**2.** In the past, we exposed most of the functions with different names for convenience. To simplify our source code, the new React specific client code exports only the following functions, listed with the necessary changes:

- `setOptions`: Not exposed anymore, use `SessionProvider` props
- `options`: Not exposed anymore, use `SessionProvider` props
- `session`: Rename to `getSession`
- `providers`: Rename to `getProviders`
- `csrfToken`: Rename to `getCsrfToken`
- `signin`: Rename to `signIn`
- `signout`: Rename to `signOut`
- `Provider`: Rename to `SessionProvider`

**3.** `Provider` changes.
- `Provider` is renamed to `SessionProvider`
- The `options` prop is now flattened as the props of `SessionProvider`.
- `clientMaxAge` has been renamed to `staleTime`.
- `keepAlive` has been renamed to `refetchInterval`.
An example of the changes:
```diff
- <Provider options={{clientMaxAge: 0, keepAlive: 0}}>{children}</Provider>
+ <SessionProvider staleTime={0} refetchInterval={0}>{children}</SessionProvider> 
```

**4.** It is now **required** to wrap the part of your application that uses `useSession` into a `SessionProvider`.

Usually, the best place for this is in your `pages/_app.jsx` file:

```jsx
import { SessionProvider } from "next-auth/react"

export default function App({
  Component,
  pageProps: { session, ...pageProps }
}) {
  return (
    // `session` comes from `getServerSideProps` or `getInitialProps`.
    // Avoids flickering/session loading on first load.
    <SessionProvider session={session}>
      <Component {...pageProps} />
    </SessionProvider>
  )
}
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working stale Did not receive any activity for 60 days
Projects
None yet
Development

No branches or pull requests

10 participants