nextjs release breaks server component use of Apollo query latest release

[kimutaiRop]

from nextjs version 13.4.6-canary.2 this package breaks when trying to use in server component
getting error

ReactServerComponentsError:

You're importing a component that needs useState. It only works in a Client Component but none of its parents are marked with "use client", so they're Server Components by default.

   ,-[/home/user/Desktop/web-test/test-frontend/node_modules/.pnpm/@apollo+client@3.8.0-alpha.15_graphql-ws@5.13.1_graphql@16.6.0_react-dom@18.2.0_react@18.2.0/node_modules/@apollo/client/react/hooks/useBackgroundQuery.js:1:1]
 1 | import { useEffect, useState, useMemo, useCallback } from 'react';
 
 The error was caused by importing '@apollo/client/index.js' in './src/graphql/home/queries.ts'.

this was working just fine before the release and then the update broke it, am thinking maybe it is caused by exporting server usable apis and only client usable in the same files

I dont know but I kinda think the issue could be somewhere around there this is caused by importing gql used to construct the query

[phryneas]

Thank you for the report!

Are you using the @apollo/experimental-nextjs-app-support package?

Also, can you please show what exactly that import looks like?

[kimutaiRop]

@phryneas phryneas
yea.. am using it
I create client by importing

import {
  ApolloClient,
  HttpLink,
  from,
  NormalizedCacheObject,
  InMemoryCache,
  split,
} from '@apollo/client';
import { onError } from '@apollo/client/link/error';
import merge from 'deepmerge';
import isEqual from 'lodash/isEqual';
import { setContext } from '@apollo/client/link/context';
import { createClient } from 'graphql-ws';
import { useMemo } from 'react';
import { RequestCookies } from 'next/dist/compiled/@edge-runtime/cookies';
import { registerApolloClient } from '@apollo/experimental-nextjs-app-support/rsc';
import { NextSSRInMemoryCache } from '@apollo/experimental-nextjs-app-support/ssr';
import { getMainDefinition } from '@apollo/client/utilities';
import { GraphQLWsLink } from '@apollo/client/link/subscriptions';

and

import { gql } from '@apollo/client';

when constructing the queries and mutations

[phryneas]

I could reproduce this with 13.4.6-canary.2, but it seems that this has been fixed in the meantime. Try 13.4.6-canary.7.

[phryneas]

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

I think it could have been fixed by a rollback in vercel/next.js#51316

I pinged them over in vercel/next.js#51341 to test my reproduction repo before merging it in again.

[huozhi]

Previously it's picking the cjs bundle that nextjs cannot analyse the imports properly, by picking up ESM bundle now we can analyze which is not expected to be used on RSC server side. apollo-lcient conatins client components imports which shoudn't be bundled on RSC server layer side as those hooks are only available in SSR and client browser.

Previously it's bundling everything even containing the stuff for client on server rendering. There're few way to workaround on it:

• ⚠️ Use const { ApolloClient } = require('@apollo/client') to still pick up cjs asset (not recommended but if you want to skip the checking temporarily)
• ✅ Use "use client" directive to marke the page as client components, it will behave properly

From what I saw from the documentation is ApolloClient needs to work with ApolloProvider which is also using react context, this is only available in client components. So it should just be marked as client component with "use client".

If apollo client is going have server components solution then it needs to have a separate react-server export condition that only contain the server only exports in apollo client that can be picked up by nextjs to work with react server bundle.

[phryneas]

@huozhi
This seems like an overzealous warning about code that will never execute on server side, and will not be actually referenced by any server component.
You can hardly expect the whole ecosystem to change their bundling to accommodate this.
Is there any way that Next applies proper tree-shaking before running this analysis?

[phryneas]

From what I saw from the documentation is ApolloClient needs to work with ApolloProvider which is also using react context, this is only available in client components. So it should just be marked as client component with "use client".

Just to clarify this:
Apollo Client also works perfectly fine in React Server components. The ApolloProvider is only required if you want to use the hooks.

[huozhi]

I see, thanks for the explaination, we'll discuss to explore the better solution for the warning. 🙏

[huozhi]

If apollo client is going have server components solution then it needs to have a separate react-server export condition that only contain the server only exports in apollo client that can be picked up by nextjs to work with react server bundle.

While we're exploring the different way to warn unexpected imports, will apollo client consider having a react-server exports in the future? Dead code elimiation is quite late phase of the bundling optimization. It's quite dangerous to mix the server components and client components code in one bundle and relying on tree-shaking. This could be a very good practice and ideal way to work with RSC.

[markerikson]

@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?

[huozhi]

@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

[markerikson]

@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.

[huozhi]

@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]

On top of that: we currently pre-bundle each of RTK's entry points: @reduxjs/toolkit, @reduxjs/toolkit/query, and @reduxjs/toolkit/query/react:

• https://unpkg.com/browse/@reduxjs/toolkit@2.0.0-beta.0/dist/query/react/

This simplifies our shipping a package that works properly with ESM imports (since we don't have to do all the nonsense around import {x} from "./someOtherFile.js" - it's all in one bundle already).

I don't know how I'm supposed to split out our package into more bundles just to satisfy Next and this requirement.

[phryneas]

Also, for extra context on the RTK-Query thing, here is a bit more of a writeup on that: facebook/react#26460

[sebmarkbage]

@markerikson I answered on the other thread. facebook/react#26460 (comment)

The user of your api can still use the same api, as long as you publish an optimized version of the inner implementation that excludes the unnecessary code branches.

[phryneas]

@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?

[sebmarkbage]

This really is part of the RSC spec and not a “linter” per se. It’s arguably just a bug that it didn’t error before. Not sure why it didn’t - it should’ve.

The workaround are fragile since any downstream dependency can change from something that accidentally doesn’t error to one that does.

Dead code elimination is not semantics and changes over time as compiler change. Sometimes to do less because they realize an optimization wasn’t safe. So it can’t be relied upon anyway. In other words, even if fixed, this might break again in a minor/patch when the compiler changes. Even if it was enough to silence the warning in some cases it’s likely you still end up with a lot of bloated unnecessary code if you don’t have two separate branches so it’s also a suboptimal runtime regardless.

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.

[phryneas]

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?

[sebmarkbage]

I don’t get why it would be a major? It would only be an update to @apollo/graphql/package.json no?

Consumers don’t have to update theirs.

[phryneas]

We don't have an exports declaration yet.
Adding an exports field to a package.json is something a lot of package maintainers (myself included) consider a breaking change (as it breaks a bunch of bundlers) - other packages have tried that in patches and minors, and rolled back from it.

[sebmarkbage]

Interesting. You’re on ESM but without exports for branching.

Most issues I’ve seen with exports is when people try to use it to add ESM but you’ve already done that step.

[phryneas]

Yes, for years - at that point, the exports field had just been added to node and pretty much nobody was considering to use it anytime soon.

(Edit: I was assuming that v3 added ESM in 2020, but it seems that even 2.0 already had ESM - back in 2017, three years before the exports field was supported by node.)

But we are barred from an exports field until our next major. We intend to do that then, and are also tracking it (#9976, #9697), but we cannot do that spontaneously.

[sebmarkbage]

Yes seems like you’re stuck behind a rock and a hard place. Maybe you can do import * in the nested files.

You can combine this with feature tests:

import * as React from “react”;

if (React.useState) {

}

Then you could optimize using the condition in the next Major.

[phryneas]

@sebmarkbage that looks almost like the workaround I had in mind when I said "evasion of static analysis" above.

What are your thoughts something on https://github.com/phryneas/rehackt ?
An intermediate package that has a "react-server" exports field with a file that re-exports all the React functions available in server components (and throwing stubs for the rest), and a a default field with a file that does a export * from 'react'?

The first would be essentially equal to what you suggest here, but the latter should make it possible to tree-shake when bundling for the browser (although I'm not sure if React even has parts that would tree-shake in any way).

[kimutaiRop]

export * from 'react' as example by @phryneas is interesting why can't react allow other packages to do what they themselves do coz I know that in my server components I can import React from "react" and not "import {useState} from "react" that means that they are exported from same file (not sure I have not checked the source code) which is exactly what apollo does and ended up giving me error in the first place coz they are just exporting from common file but not that it forces us to use hook depended function and providers

[markerikson]

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.

[phryneas]

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.

[sebmarkbage]

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.

[leerob]

For clarity, the Next.js PR and canary version mentioned in the original message was reverted: vercel/next.js#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.

[benkingcode]

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.

[phryneas]

@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.

[phryneas]

@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 :)

[phryneas]

This seems to be addressed by vercel/next.js#56501 .

The only question still open is if that PR also addresses #11167 - in that case we could close this issue.