apollographql / apollo-client-nextjs

Apollo Client support for the Next.js App Router
https://www.npmjs.com/package/@apollo/experimental-nextjs-app-support
MIT License
436 stars 32 forks source link

New package (0.11.0+) brings Apollo Client into Browser bundle when not used in Client Components #341

Open lionskape opened 1 month ago

lionskape commented 1 month ago

My project uses CommonJS (no "type": "module"). I found an issue.The new version of @apollo/experimental-nextjs-app-support have been added to a bundle, on pages where there is no ApolloProvider, only RSC calls of apollo client.

Steps to reproduce:

alessbell commented 1 month ago

Hi there @lionskape 👋

What you're describing indeed sounds very strange. My first instinct is that a question of including server code in a client bundle would be best asked of the framework/bundler.

What version of Next.js are you using? Did you notice this change after this package, or multiple dependencies? Any additional information you can provide would be very helpful in determining where this issue would be best directed. Thanks!

lionskape commented 4 weeks ago

@alessbell hi!

I believe that it is just apollo client, which can be used on SSR (but I do not need it). I'm using nextjs@14.2.5 with webpack (without customizations).

My case: app/my-page/page.tsx (server component, no "use client") -> const {data} = await(await getClient()).query({query: MyQuery});. No more queries, no ApolloProvider.

"@apollo/experimental-nextjs-app-support": "0.10.1" Everything works fine. Сlient bundle does not contains apollo client.

"@apollo/experimental-nextjs-app-support": "0.11.2" Everything works, but in client bundle there is a lot of new unused code - @apollo/client and something extra from @apollo/experimental-nextjs-app-support

jerelmiller commented 4 weeks ago

Hey @lionskape 👋

0.11 added a new shared entry point that now uses export conditions in order to determine the right bundle to use (assuming you're importing from the top-level entry point). This should work well with Next.js and the app router as this is the bundling strategy React recommends for RSC architecture, though the sub imports (i.e. the /rsc, /ssr entry points) remained unchanged.

Can you put together a reproduction for us? A reproduction would go a long way to figuring out why you're seeing the bundle increase.

phryneas commented 4 weeks ago

I can reproduce it here: https://github.com/phryneas/apollo-client-nextjs-reproduction-341/commit/8b39385ac546e1e3d5b5921bd758ef3bbe237724

This is clearly a bug in Next.js. It should not bundle any library that's only referenced in RSC - no matter how we build, no matter how we bundle, it should not have any influence on that.

phryneas commented 4 weeks ago

https://github.com/vercel/next.js/issues/68972

lionskape commented 4 weeks ago

@phryneas thank you for reproducing an issue!

I believe they will not include a fix in 14th release. May be it is possible to make workaround in this integration library? Something like additional dedicated @apollo/experimental-nextjs-app-support/rsc export point, with only rsc parts?

phryneas commented 4 weeks ago

Let's wait for their feedback first. This is an immense bug that's at the core of the promise of RSC - not shipping server libraries to the client. We don't even know the internals on how it is triggered on their side at this point.

jmikrut commented 3 weeks ago

Hey @phryneas just a heads-up, we had this same issue at Payload and turns out it's a tough problem to solve.

https://github.com/vercel/next.js/issues/50285

If you have any files marked with use client then they're going to show up in the client side bundle regardless of intent. Is that what's happening in your case I wonder?

phryneas commented 3 weeks ago

Yeah, I had similar thoughts occur to me over the weekend - we have a RSC call that creates a component that references a client component.

I could reduce the impact with a well-placed and lazily executed React.lazy over in #345, so while I still believe that this is a bundling problem on the Next.js side, we can work around it now.

phryneas commented 3 weeks ago

@lionskape could you please verify if this fixes the issue for you?

npm i @apollo/experimental-nextjs-app-support@0.0.0-commit-release.0.1724059607.3b1a1ef
lionskape commented 2 weeks ago

@phryneas sorry for being late. Thank you, great job!

I have validated your variant, so:

Maybe you can just create a separate entrypoint for rsc, without any "use client" imports in the import graph? Looks like NextJS team does not treat this issue as a bug.

phryneas commented 2 weeks ago

@lionskape we're probably not going to get around that stub unless Next.js does something on their side.

Maybe you can just create a separate entrypoint for rsc, without any "use client" imports in the import graph?

We had separate entry points, and it has confused users a lot. I fear that the moment we have that entrypoint, even if we don't recommend using it, a lot of users will pick it up - in large parts because LLMs just recommend bonkers code that "looks right" to users, but lacks nuance.

I think having that separate entrypoint would do more harm than good - especially since we want most users that use RSC to use the PreloadQuery component, and we'd have to drop that from that separate entrypoint.

It's good to know that this already brings a significant improvement, so I'm going to merge and ship that PR, it will benefit a lot of users. Thank you for bringing this up and testing it out!