apollo-client: nextjs release breaks server component use of Apollo query latest release

Just read through the last few hours of comments and I have to say I’m pretty frustrated.

I’ve spent months trying to upgrade the Redux family packages just to get proper exports config and ESM support. Every time I make a change, someone else pops up to tell me I’m doing it wrong or there’s another edge case I need to be aware of. I finally got a set of configurations that seem to reasonably work (although even there Andrew Branch from the TS team is telling me there’s edge cases around moduleResolution: "node16" and .mjs/.d.ts mismatches).

Now you’re telling me that I have to spend more time coming up with more bundle configurations, just to keep my code from breaking in an RSC environment, and there’s no real documentation or proper examples on what we need to do to keep things working. For that matter, the suggested import workaround seems pretty bizarre.

This is extremely demoralizing 😦

From my perspective, there is a huge amount of churn going on with React right now, and little to no documentation or help being given to the ecosystem to deal with it. I realize a lot of this stuff is still WIP, but this is putting a ridiculous amount of stress on me (and presumably other library maintainers) just trying to keep up with what’s going on.

I don’t have a particular solution to offer atm, but you should know I’m pretty upset by this.

At this point, I think it’s reasonable to start asking when the RSC experiment is going to be cancelled. You are introducing so much uncertainty, churn and complexity into the ecosystem… for what? The purported benefits absolutely do not make up for this much chaos.

@leerob the messaging here is continuously switching back and forth between “this was reverted” and “we will reintroduce this, as this is how it should have worked from the start”, so please, when you have found a direction you think you want to settle on, bring us in on the discussion again, let’s test that together and make sure it works for everyone (or that there is at least a consistent workaround) - and please do so early enough that we have enough time to implement & release necessary (possible) changes on our side before you ship and we get flooded with bug reports 😃

I’m gonna +1 on what Mark said here, and want to raise one additional concern: what React encourages here (and does itself) is to have an export that sometimes, depending on the environment, is absolutely not typesafe.

TypeScript will tell me that createContext is there, just like useState, useEffect and all the others. In some execution context it then just isn’t. And now we have a compiler in place that will, in addition to TypeScript, essentially try to guard around that.

And this is a decision that React made for itself, which is kinda fine, but essentially you are now going around and start encouraging other libraries to potentially also remove parts of their public interface and let users discover on accident that things they just imported are undefined as long as they are in a React Server Component.

I mean, of course, we can still have those imports there and at least return functions that will throw on execution, or something like that (and we will definitely be doing that instead of just undefineding parts of our public interface!) - but this cascade effect that is coming up here just seems incredibly weird to me.

I mean you don’t have to have the same interface for both. You can also just have two different entry points for the two environments or one shared and one extra for client. That’s a design decision for the library.

The main intention is for it to have different implementations with the same API.

The way React does it, by using static named exports, you still get early build errors which are just as good as TypeScript errors.

That said, it’s pretty silly that TypeScript doesn’t yet support multiple environments. Like Buffer.from(…) or import “fs” is not type safe depending on if the code is meant to Node or not but we currently just assume all types are available in any JS environment. Same thing. Ideally TypeScript could check multiple environments in one pass.

@sebmarkbage This would be forcing every ecosystem package to add an exports field to their package.json - that is a breaking change with many bundlers out there and needs to be released in a major. You can’t possibly expect every npm package that should keep working in RSC to bump up a major for just that, while you plan to put a change like this out in a patch version.

Also, even apart from that, this is not a minor thing, it requires to completely revamp the packaging setup, with the risk of breaking other things. This puts an enormous amount of churn on the ecosystem.

I’d love to turn around the question here: in which way does it hurt that these hooks are in the bundle? As long as they are not executed, they don’t have any consequences - and if they are, it’s a runtime error that would usually already occur during bundling.
Would a possible compromise be that a package adds some meta field in their package.json to opt out of this check? (This is still bad enough, but doable in a patch release.) If you really wanted, you could show information along the lines of “package X has opted out of compilation-time checking, use at your own risk”.

That said, the real solution would still be that you don’t validate all touched files, but apply some kind of dead code elimination. In the case of Apollo Client, the hooks are in completely different files than the imported values - they are just re-exported by a common parent file.
This is a build step anyways, and you are already applying dead code elimination to the browser bundle, so why not the server bundle too?

For clarity, the Next.js PR and canary version mentioned in the original message was reverted: https://github.com/vercel/next.js/pull/51316. This was a bug, and is not included on the stable release channel (a patch release just went out).

It’s possible to run into errors when running the Next.js canary channel, since we’re landing changes quickly and testing things out. These things get stabilized before moving out of the canary channel. This is very helpful as it allows us to catch issues early while dogfooding. So I’m glad this helped us catch another issue!

We appreciate the feedback here and will make sure we consider your feedback as we roll out a proper PR here.

@dbbk I know we all are here for the Twitter drama, but can we please keep the issue tracker on topic here? 😄

This issue is about potential ways Nextjs might be changing how their bundler interprets external packages.
I’m very fine having this discussion for Apollo Client and other packages like Redux here, as that means we don’t have to have this discussion in parallel in many different issue trackers (and don’t miss any crucial details - more eyes see more), but please let’s keep it to that general topic.

@huozhi : that’s definitely a problem. RTK Query is designed with two separate entry points: one that is entirely UI-agnostic and has no knowledge of React at all, and another that is React-specific and automatically generates hooks for each declared endpoint.

We’re still trying to work out some of the intended use cases and approaches, but it’s reasonable from our perspective that a user might want to declare an RTKQ API definition using the React-specific entry point, using the plain data fetching logic it exposes within an RSC, and also import that same API definition into a client component to fetch via hooks. This should only be a runtime issue if they tried to actually use the hooks in an RSC, but it would be frustrating for Next to throw an error even if they’re not being used, just imported by RTKQ.

bloated unnecessary code if you don’t have two separate branches so it’s also a suboptimal runtime regardless.

That’s exactly what compilers are for. We write code with focus on tree-shakability, compilers do the tree-shaking. And that will always be more efficient than us manually trying to foresee what users could want to use.

Most code out there written specifically for client React just won’t work in a Server Component which is why early errors are important. Most libraries need to be designed with Server Components support in mind at which point there’s a change to be made where this can be addressed. That’s an opportunity to add the fork.

Yes, and that library you are talking about there is @apollo/experimental-nextjs-app-support, which has Apollo Client as a dependency.

The part of Apollo Client we are talking about here is not written with React in mind at all - there is a small part of Apollo Client that is written for React, correct, but the core is also used in various other packages.
You cannot expect us to release a 4.0 and force our Angular users to migrate to that because the React team wrote something into an RFC.
We still have people migrating from Apollo Client v2 to v3, and v3 has been released in 2020. Doing a major version bump is something that will always leave a bunch of users behind that will not receive updates afterwards, and once we do that, it should actually be beneficial for a majority of our users, not only churn for most of them. At this rate, I would rather foresee us add some evasion to static analysis to the package, and that can really not be what you are after?

@markerikson Thanks for the explanation, it’s super helpful! 👍 We’ll move to the direction that only erroring the used ones. The change was only landed in a canary release and reverted already. Will improve the detectors first then to re-land the change that erroring on apollo-client

@markerikson it would contain the code that will only run inside server components, excluding all the code which will run on client components. For example, hooks with useState won’t be there, or any code that is running in SSR and browser also won’t be there

@huozhi : what is a "react-server" bundle supposed to actually contain, code-wise? What should be different than any of the other bundles that get shipped in a package to NPM?

This seems to have been fixed between canary.5 and canary.6

I think it could have been fixed by a rollback in https://github.com/vercel/next.js/pull/51316

I pinged them over in https://github.com/vercel/next.js/pull/51341 to test my reproduction repo before merging it in again.