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 Import of Client Component from Server Component Not Code Split #61066

Open
ajwootto opened this issue Jan 24, 2024 · 10 comments
Open

Dynamic Import of Client Component from Server Component Not Code Split #61066

ajwootto opened this issue Jan 24, 2024 · 10 comments
Labels
bug Issue was opened via the bug report template. Lazy Loading Related to Next.js Lazy Loading (e.g., `next/dynamic` or `React.lazy`).

Comments

@ajwootto
Copy link

ajwootto commented Jan 24, 2024

Link to the code that reproduces this issue

https://github.com/ajwootto/nextjs-dynamic-loading-repro

To Reproduce

  1. yarn and start the repro server with yarn dev
  2. observe that the "DynamicClientComponent" is only ever dynamically imported using the Next.js dynamic helper, and is never rendered.
  3. Visit the root page on localhost and check the network tab
  4. view the contents of page.js
  5. Observe that the implementation code for DynamicClientComponent is present in that bundle. Search for the words "Dynamic Component That Should Not Be Here" to see it.

Another case using Suspense

  1. Navigate to /suspend
  2. Observe that there's a suspense fallback rendered which is then replaced with the dynamically loaded client component
  3. Check the initial page.js file again for the same string. It is present in that bundle still.

Current vs. Expected behavior

Current: A client component that is dynamically imported by a server component is included in the initial client bundle, regardless of whether it is rendered or not during server rendering. Even if it is rendered inside a Suspense boundary it is still included in the initial bundle.

Expected: The client bundle should not contain any client component code which is dynamically imported but not rendered. It should also not include any client component code that is dynamically imported and rendered inside a Suspense boundary from a server component. The implementation of that component should be streamed later to the client.

The repro example is the simplest possible case where the imported component is never called no matter what, but I would also expect something like this to work:

const ConditionalComponent = dynamic(() => import('./DynamicClientComponent'))

export default function Home() {
    // some arbitrary condition which could be determined dynamically (think feature flags etc.)
    const shouldRender = false
    // ConditionalComponent should not be sent since on this server render pass it was never rendered
    return  shouldRender && <ConditionalComponent/>
}

Provide environment information

Operating System:
  Platform: darwin
  Arch: arm64
  Version: Darwin Kernel Version 23.2.0: Wed Nov 15 21:53:18 PST 2023; root:xnu-10002.61.3~2/RELEASE_ARM64_T6000
Binaries:
  Node: 20.10.0
  npm: 10.2.3
  Yarn: 3.6.2
  pnpm: 8.6.12
Relevant Packages:
  next: 14.1.1-canary.7 // Latest available version is detected (14.1.1-canary.7).
  eslint-config-next: N/A
  react: 18.2.0
  react-dom: 18.2.0
  typescript: 5.1.3
Next.js Config:
  output: N/A

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

App Router, Dynamic imports (next/dynamic)

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

next dev (local), next build (local), next start (local)

Additional context

Not sure if this is just a docs or understanding issue, or whether what I think should be the behaviour here is impossible, but it really seems intuitive to me that a dynamic import should not be included in the initial client bundle if its not called during render.

@ajwootto ajwootto added the bug Issue was opened via the bug report template. label Jan 24, 2024
@github-actions github-actions bot added the Lazy Loading Related to Next.js Lazy Loading (e.g., `next/dynamic` or `React.lazy`). label Jan 24, 2024
@jgardner3-chwy
Copy link

I'm seeing this same thing on 14.0.4. It only works properly when you use dynamic in a component marked with 'use client' or it's children. Doesn't split the chunk if dynamic is used in an RSC to load a client component.

Did you ever figure out a workaround or fix? I'd rather not put a separate client component wrapper around everything i want to split and dynamically load.

@ajwootto
Copy link
Author

ajwootto commented Mar 4, 2024

I did not figure out a fix no, I've just had to be careful to put the dynamic loading in client components.

It's extra annoying for us though because we're publishing a feature flag SDK that supports conditional dynamic loading based on flags, but currently the helper can only be used in a client component otherwise you risk accidentally sending the unrendered code to the client. For most people that's just a performance concern but for our use case its a risk of potentially leaking unreleased features to people that aren't supposed to see them yet, so it's kind of a big pitfall.

@ryami333
Copy link

ryami333 commented Mar 5, 2024

Experiencing the same thing after migrating to app-dir. We have a "modular" website, where an editor can compose a page from 50 different block-types. In NextJS, we have some code which looks like this:

// app/page.tsx
export default function Page() {
  return (
    <>
      {blocks.map(block => <Switch block={block} />
    </>
  );
};
// components/Switch.tsx

export function Switch({ block }) {
  switch (block.type) {
    case "rich-text": {
      const RichText = dynamic(import("../components/RichText.tsx"));
      return <RichText content={block.content} />
    }
     case "image": {
      const Image = dynamic(import("../components/Image.tsx"));
      return <Image src={block.src} />
    }
    case "contact-form": {
      const ContactForm = dynamic(import("../components/ContactForm.tsx"));
      return <ContactForm fields={block.fields} />
    }
    // … etc (times 50)
};

I can't tell you how disappointed I was (after spending hours on migrating to the app-dir) that my client bundle now includes all 50 components on every page.

@JohnRPB
Copy link

JohnRPB commented Mar 11, 2024

It's frustrating to know that we could be receiving code-split client components on first byte, taking advantage of parallelization, but instead have to wait for the application to build and call the server again. It's a noticeable delay.

@markoleavy
Copy link

markoleavy commented Apr 5, 2024

The only solution I've found - but I tell you now, it's a DX nightmare - is wrapping all client components in a wrapper, like that:

// page.ts

const Index = async () => {

  const { isEnabled } = draftMode();
  const { homepage} = await getHomeProps();

  if (!isEnabled) {
    return <Home props={homepage} />;
  } else {
    return <PreviewWrapper props={homepage} />;
  }
};
export default Index;

// PreviewWrapper.ts
'use client';

const PreviewComponent = dynamic(() => import('./PreviewComponent'), {
  loading: () => <Loader/>,
});

const PreviewWrapper: React.FC<{ props: HomepageProps }> = props => {
  return <PreviewComponent {...props} />;
};

export default PreviewWrapper;

export default Index;

We also have like 50 modular components (and a hundred of svg icons!!), and using that method would mean exporting the props type of every component, and writing a wrapper for each of them.

I'm trying to write an HOC or something similar that could do that routing for different components, but dynamic() doesn't accept variables, and might be really difficult to type different props (never used generics for components, but that could be the case).

@markoleavy
Copy link

Something like that:

// ServerDynamicImporter.tsx
'use client';
import dynamic from 'next/dynamic';
import React from 'react';

import Loader from './Loader';
import { PreviewWrapperProps } from './PreviewWrapper';

type Props<T> = {
  props: T;
  component: 'PreviewWrapper' | 'ButtonHeroWeb' | 'SearchMonth';
};

const PreviewWrapper = dynamic(() => import('./PreviewWrapper'), {
  loading: () => <Loader backgroundColor="secondaryLighter" />,
});

const ButtonHeroWeb = dynamic(() => import('../layout/ButtonHeroWeb'));
const SearchListing = dynamic(() => import('./SearchMonth'));

const ServerDynamicImporter = <T,>({ props, component }: Props<T>) => {
  switch (component) {
    case 'PreviewWrapper':
      return <PreviewWrapper {...(props as PreviewWrapperProps)} />;
    case 'ButtonHeroWeb':
      return <ButtonHeroWeb {...(props as { children: string })} />;
    case 'SearchMonth':
      return <SearchListing {...(props as { children: string })} />;
  }
};

export default ServerDynamicImporter;

Then invoked:

// AnyServerComponent.tsx

export const AnyServerComponent: React.ReactNode = () => {
   return (
      <div>
         <ServerDynamicImporter<{ children: string }>
            component={'ButtonHeroWeb'}
            props={{ children:'Button content'}}
         />
         <ServerDynamicImporter<PreviewWrapperProps>
            component="PreviewWrapper"
            props={{ page: home }}
         />
      </div>
   );
}

@WenChun19
Copy link

The only solution I've found - but I tell you now, it's a DX nightmare - is wrapping all client components in a wrapper, like that:

// page.ts

const Index = async () => {

  const { isEnabled } = draftMode();
  const { homepage} = await getHomeProps();

  if (!isEnabled) {
    return <Home props={homepage} />;
  } else {
    return <PreviewWrapper props={homepage} />;
  }
};
export default Index;

// PreviewWrapper.ts
'use client';

const PreviewComponent = dynamic(() => import('./PreviewComponent'), {
  loading: () => <Loader/>,
});

const PreviewWrapper: React.FC<{ props: HomepageProps }> = props => {
  return <PreviewComponent {...props} />;
};

export default PreviewWrapper;

export default Index;

We also have like 50 modular components (and a hundred of svg icons!!), and using that method would mean exporting the props type of every component, and writing a wrapper for each of them.

I'm trying to write an HOC or something similar that could do that routing for different components, but dynamic() doesn't accept variables, and might be really difficult to type different props (never used generics for components, but that could be the case).

That is a good solution. But it opts all the dynamically imported components into client components, which might affect the rendering performance. This is the solution that is not so preferable when we have no choice but to reduce initial bundle size. Both sides have their good and bad things the way i see it. Hope this dynamic import in RSC behavior can be clarified or fixed as soon as possible.

@markoleavy
Copy link

markoleavy commented Apr 16, 2024

That is a good solution. But it opts all the dynamically imported components into client components, which might affect the rendering performance. This is the solution that is not so preferable when we have no choice but to reduce initial bundle size. Both sides have their good and bad things the way i see it. Hope this dynamic import in RSC behavior can be clarified or fixed as soon as possible.

@WenChun19 this is supposed to be used for components that already are marked as 'client components'. Server components aren't sent to the client anyway, and "hybrid component" (server components imported by client components) are imported dynamically if the component that import them is dynamically imported. So from this point of view there is no difference in performance.
That said, I managed to reduce my initial bundle for home page from this:

Screenshot 2024-04-05 at 17 05 29

To this:

Screenshot 2024-04-05 at 16 49 35

Didn't get any better performance, nor different "page weight" size, according to pagespeed and webpagetest.

@salos1982
Copy link

I have similar issue in 14.2.4 (canary 15 version also have the same issue) and found only one solution that was mentioned earlier (use wrapper client components).
I agree that it is very strange behavour and it takes me 3 days to understand what is going with next/dynamic.

@Alt-er
Copy link

Alt-er commented Sep 22, 2024

Any progress?

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. Lazy Loading Related to Next.js Lazy Loading (e.g., `next/dynamic` or `React.lazy`).
Projects
None yet
Development

No branches or pull requests

8 participants