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

Unused client components are added to a client's bundle in app router. #50285

Closed
1 task done
r00dY opened this issue May 24, 2023 · 10 comments
Closed
1 task done

Unused client components are added to a client's bundle in app router. #50285

r00dY opened this issue May 24, 2023 · 10 comments
Labels
area: app App directory (appDir: true) bug Issue was opened via the bug report template. locked

Comments

@r00dY
Copy link

r00dY commented May 24, 2023

Verify canary release

  • I verified that the issue exists in the latest Next.js canary release

Provide environment information

Operating System:
      Platform: darwin
      Arch: arm64
      Version: Darwin Kernel Version 22.3.0: Mon Jan 30 20:38:43 PST 2023; root:xnu-8792.81.3~2/RELEASE_ARM64_T8112
    Binaries:
      Node: 16.18.1
      npm: 8.19.2
      Yarn: 1.22.17
      pnpm: 8.1.0
    Relevant packages:
      next: 13.4.4-canary.8
      eslint-config-next: N/A
      react: 18.2.0
      react-dom: 18.2.0
      typescript: 5.0.4

Which area(s) of Next.js are affected? (leave empty if unsure)

App directory (appDir: true)

Link to the code that reproduces this issue or a replay of the bug

https://github.com/r00dY/app-router-test

To Reproduce

  1. npm run build
  2. npm run start
  3. Go to the homepage.
  4. Check out JS sources and see what happens: UnusedClientComponent is added to the client bundle:
Screenshot 2023-05-24 at 17 34 59

Describe the Bug

UnusedClientComponent shouldn't be included in the client bundle and yet it is.

What happens in more detail:

  1. page.tsx is a server component.
  2. It imports UnusedClientComponent. UnusedClientComponent is used (it's assigned to an object property).
  3. But it's used only in a server environment! UnusedClientComponent is never passed to client component (it's missing from serialized props for HeaderClientComponent). Nor it is imported by some other client component that is already included in a client bundle.
  4. And yet, it's added to the clients bundle. It's obviously wrong.

Expected Behavior

If a client component reference is used in server component and meets 2 extra conditions:

  1. It's not passed via props to a client component.
  2. It's not directly imported and used by other client component included in a clients bundle.

Then it should be excluded from a clients bundle.

Which browser are you using? (if relevant)

Chrome 113.0.5672.126 (Official Build) (arm64)

How are you deploying your application? (if relevant)

Locally

@r00dY r00dY added the bug Issue was opened via the bug report template. label May 24, 2023
@github-actions github-actions bot added the area: app App directory (appDir: true) label May 24, 2023
@r00dY r00dY changed the title Unused client components are added to a bundle in app router. Unused client components are added to a client's bundle in app router. May 24, 2023
@jmikrut
Copy link
Contributor

jmikrut commented Apr 16, 2024

We're also seeing this as an issue with Payload. Exactly as OP described. I am open to working around the logic within Next.js's compiler if there is an option to do so for sure but so far we haven't found a suitable workaround. Will post back here if we get something working.

@jmikrut
Copy link
Contributor

jmikrut commented Apr 17, 2024

My team and I have been doing some thinking on this issue and we imagine it’s going to be pretty difficult to determine that an imported, but not rendered, client component should NOT be added to the client JS bundle.

I can also see that this is likely a fringe case that only certain packages will run into.

We've thought of a few much simpler avenues.

Option 1 - magic import comments

Like Webpack's magic comments, we could allow specific dynamic imports to be tagged with their own directive, which from that point on, would mark all of their client component imports as unnecessary to be sent to client JS.

Something like this:

const serverOnlyDep = await import(/* serverOnly: true */ './config')

This would solve for 100% of the things that we're running into I believe, and could also be done in a performant way within Webpack / Turbopack. The thing is, in our case, this specific dependency does indeed need to have its client dependencies added to the page's client bundle, but in other cases, it does not. Same file, different usage.

Option 2 - a new Next.js config property

We could have a new Next.js config option, similar to serverComponentsExternalPackages, but for passing in identifiers (relative or absolute path, package name) to disable this automatic addition of client files for any sub-modules.

With that second approach, we'd be able to set up some type of proxy file that imports, and then re-exports the module in question. We could then add its proxied path to this new Next.js config property.

const nextConfig = {
  experimental: {
    serverOnlyDependencies: [ '/Path/to/config-proxy.ts']
  }
}

Then we'd have a simple wrapper like this:

// /Path/to/config-proxy.ts
import config from './payload.config'

export default config

So when we import the config proxy, the addition of any server actions / client components will be disabled across the board.

If either of these seem like a decent idea, and if someone can point me to the right place for this, my team would be happy to contribute!

@feedthejim
Copy link
Contributor

@jmikrut have you looked into https://www.npmjs.com/package/server-only ?

@jmikrut
Copy link
Contributor

jmikrut commented Apr 22, 2024

Hey @feedthejim yep I did some testing with that but it had no effect. If I understand that package right, all it’ll do is throw an error if one of the dependencies containing it is loaded within a client file, right?

Does Next.js do anything specific within its compiler related to that package?

To clarify, I put that package at the top of our Payload config (the thing that imports, but does not render resources from client files). We import the Payload config in server components, but never in client files. So no error was even thrown.

But, all client sub-dependencies of the Payload config still get automatically bundled into the corresponding client-side JS for the given page.

@feedthejim
Copy link
Contributor

could you maybe provide a specific repro that outlines the scenario?

@jmikrut
Copy link
Contributor

jmikrut commented Apr 22, 2024

The repo that OP provided is the exact same scenario as ours. Note that he binds his UnusedClientComponent to an object, but does not render it.

Here is the line that I am referring to:
https://github.com/r00dY/app-router-test/blob/main/src/app/page.tsx#L8

So, even though UnusedClientComponent is indeed imported and referenced in the server component, it is not meant to make its way to the client bundle. But, it does.

That is the exact issue we're having. If you do need an additional reproduction, I would be happy to make one! But it would be basically exactly the same as the @r00dY one above.

Let me know!

AlessioGr added a commit to jmikrut/next.js that referenced this issue May 7, 2024
AlessioGr added a commit to jmikrut/next.js that referenced this issue May 8, 2024
@jmikrut
Copy link
Contributor

jmikrut commented May 13, 2024

Hey @feedthejim — I've got a PR open that addresses this issue. I would love to hear your teams' thoughts! Anything I can do?

@gustavotoyotarh
Copy link

gustavotoyotarh commented Sep 26, 2024

This problem is erasing 24 points in the PageSpeed score of websites using our CMS. Any solution?

@jmikrut
Copy link
Contributor

jmikrut commented Nov 20, 2024

OK so in the end, after trying to think about this a thousand different ways, we actually determined that this was kind of a fundamental flaw with how Payload was doing things.

We were importing, and referencing, client components in a file. In the bundler world, it makes sense that those files would be included in the output bundle because we did indeed reference them (bound them to objects). If we didn't reference them, then they would be omitted from the bundle.

We ended up coming up with an alternate pattern entirely.

Rather than importing and referencing client components like this:

import { MyClientComponent } from './MyClientComponent'

export const config = {
  // The act of binding the component to the object tells bundlers
  // that it is indeed important and should be bundled
  component: MyClientComponent
}

We updated Payload to accept only the path to the components that we need in our config like this:

export const config = {
  // Now bundlers wouldn't even know this is a file
  component: './MyClientComponent'
}

And then we dynamically build up an import map from those paths, so we can import that when we need it, rather than having the client-side files imported into the config at all.

This actually had a lot of other benefits, too, because now our config is 100% Node-safe and is smaller, importing less files, which leads to faster compilation overall. So now we can allow our users to import their Payload configs in any Node environment without having to handle JSX, image imports, CSS, or any other wild client-side stuff.

Kind of a win in the end. I'd say my interest in this issue is now completely resolved and in the end it was an "us" thing. I think we're better off for now after having handled it in a different way.

Copy link
Contributor

github-actions bot commented Dec 5, 2024

This closed issue has been automatically locked because it had no new activity for 2 weeks. If you are running into a similar issue, please create a new issue with the steps to reproduce. Thank you.

@github-actions github-actions bot added the locked label Dec 5, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: app App directory (appDir: true) bug Issue was opened via the bug report template. locked
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants