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

dynamic() ignores <Suspense> hierarchy resulting in layout flicker #73830

Open
ninja- opened this issue Dec 12, 2024 · 2 comments
Open

dynamic() ignores <Suspense> hierarchy resulting in layout flicker #73830

ninja- opened this issue Dec 12, 2024 · 2 comments
Labels
bug Issue was opened via the bug report template.

Comments

@ninja-
Copy link

ninja- commented Dec 12, 2024

Link to the code that reproduces this issue

https://codesandbox.io/p/devbox/determined-firefly-9dq24w

To Reproduce

Open codesandbox on main page and observe the flicker

Current vs. Expected behavior

My app suffers from a rare (when on prod-optimized build) race condition, which results in layout flicker when navigating between pages.
It got more intense as I moved more pages out of SSR to improve performance, using dynamic.
I recalled an earlier issue in which the Router could make a full browser page refresh under some conditions resulting in a white-page flicker, but looks like that issue is long gone (with some NextJS update) and I wasn't able to race it that way.

With Chrome frame-by-frame recording, I finally raced it and seen that the flickered frame contains just the <Footer element without the webpage content.

I wrapped my whole layout code with <Suspend, but the issue remained

So the layout code was

const Layout: FC<AppProps> = ({ Component, pageProps }) => {
  return (
    <Suspense>
      <div>
        <Header />
        <Component {...pageProps} />
        <Footer />
      </div>
    </Suspense>
  );
};
export default Layout;

Why?
NextJS uses <Suspend in dynamic(), but it will always force fallback to be either a provided element, or it's own element which returns empty content when Suspended.
That means React can't see the parent <Suspend which only works when the child has fallback=null, which would avoid rendering incomplete component with just <Footer inside.

I don't see rendering "empty" as a default fallback making sense, IMO it would make sense to use no fallback= at all and refer the error handling, which is part of current "fallback" function, to the ErrorBoundary, and otherwise just properly Suspense.

Alternatively, I tried forcing loading to undefined which should override it properly, but there's another part of code that expects it to be valid and results in crash(it switches the implementation file between server and client, or something):

https://github.com/vercel/next.js/blob/canary/packages/next/src/shared/lib/loadable.shared-runtime.tsx#L142

EDIT: I double checked and what I wrote about <Suspend fallback={null} is not right because it seems have no special mechanics compared to a fallback function that itself returns a null component. So in that case, mechanics of lazy components seem enough without <Suspend, and if the crash when loading: undefined is fixed, and logic for hasSuspenseBoundary is extended to avoid forcing it without SSR, it can be used to workaround this problem.

https://github.com/vercel/next.js/blob/canary/packages/next/src/shared/lib/lazy-dynamic/loadable.tsx#L52C11-L52C30

Provide environment information

"next": "15.1.1-canary.1",
    "react": "^19.0.0",
    "react-dom": "^19.0.0"

Which area(s) are affected? (Select all that apply)

Not sure

Which stage(s) are affected? (Select all that apply)

next dev (local), next start (local)

Additional context

No response

@ninja- ninja- added the bug Issue was opened via the bug report template. label Dec 12, 2024
@r34son
Copy link
Contributor

r34son commented Dec 26, 2024

@ninja- Facing same issue. Could you please explain which workaround you found?

@ninja-
Copy link
Author

ninja- commented Dec 26, 2024

@r34son if you copy the footer to each page individually, and assuming header is just <head> tags - it should make things better - I think it would change the layout flicker to a minor whitepage flash instead. But I didn't want to deploy a forked NextJS version, so I put this on hold as-is

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issue was opened via the bug report template.
Projects
None yet
Development

No branches or pull requests

2 participants