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
343 stars 23 forks source link

Static fragments on Client Components #27

Closed amannn closed 1 month ago

amannn commented 10 months ago

Hey! Really exciting to see Apollo experimenting with RSC support in Next.js! šŸ‘

I'm using a Relay-esque approach with Apollo Client, where components define fragments so that data requirements can be colocated with the code that consumes them and to reliably orchestrate data dependencies upwards a component hierarchy.

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

export default function User({user}) {
  return <p>{user.name}</p>
}

User.fragments = {
  user: gql`
    fragment User_user on User {
      name
    }
  `
}

I noticed this works as long as the components that define fragments are Server Components, but since imports from Server Components to Client Components create a lazy reference, the static fragments are no longer resolvable.

Based on one of the examples in this repo, I've added this commit for demonstration: https://github.com/amannn/apollo-client-nextjs/commit/fa8c9d381c01f161a5c204152a317e608086581d

This is not really a limitation of Apollo, but of the RSC model I guess. I was wondering if this is something that you're interested in supporting and if there's something that can be done about this? The only thing that comes to my mind is a compiler that resolves the static parts at build time, but I guess ideally there would be 1st-class support from the RSC bundler for this.

Many thanks! šŸ™Œ

phryneas commented 10 months ago

Hi there, I love the experimentation and the PR :)

Fragment colocation is definitely something we want to give some more of a platform. I'll add some unstructured thoughts in here, but I think @jerelmiller might have more input here.

jerelmiller commented 10 months ago

@amannn first of call, I love this pattern and highly recommend it so I'm happy to see you using it. In fact, its a pattern I use all over our Apollo Spotify demo. While we recommend the separation between client and server components, I think fragments make a ton of sense for client components to declare their data needs so that server components know what to include in their queries. This provides a nice data handoff between server and client components.

I was surprised that static members don't resolve properly, but I guess that would make sense if those are lazy. I'm curious, do you know how non-default exports are resolved? My first intuition was to just export const fragments from your component rather than adding those as a static member, but I'm not sure if you run into the same problem. I'm having difficulty finding anything on this topic both in the Next.js docs and Google searching. I suppose I could spin up a small app myself to try it but I've stopped short of that šŸ˜œ.

Regardless, I would love to explore this a bit more and allow this pattern if its something Apollo itself can provide. This pattern is super powerful and I encourage it as much as we can (in fact, I'd love to make sure our docs emphasize this pattern much more than they do). I don't know all the limitations we run into with bundlers and whatnot though so this could be difficult.

Thanks for raising this concern!

amannn commented 10 months ago

Thank you @phryneas and @jerelmiller for your quick replies! Really encouraging to hear that you're putting more focus on colocated fragments, I think especially with TypeScript being pretty much an industry standard by now, this is a really good call!

My first intuition was to just export const fragments from your component rather than adding those as a static member, but I'm not sure if you run into the same problem.

Unfortunately, that causes the same problem!

Everything imported from a client boundary is available as [Function (anonymous)] in the importing module. Calling the function results in:

Error: Attempted to call fragments() from the server but fragments is on the client. It's not possible to invoke a client function from the server, it can only be rendered as a Component or passed to props of a Client Component.
  • I think the separation between client components and server components (and their fragments) is mostly a good thing. We give this advice in the readme:

Hmm, I read this part in the docs. I'm wondering from which perspective this is desirable? Is this seen as a necessity to work within the technical constraints we have or does it solve a problem for users or developers?

Here's a practical example for a component that can be used for updating a user profile:

'use client';
import CountrySelector from './CountrySelector';

function HighlyDynamicUserProfile({countries, user}) {
  const [country, setCountry] = useState(user.country);

  // ...

  return <>
    {/* ... */}
    <CountrySelector countries={countries} value={country} onChange={setCountry} />
  </>
}

HighlyDynamicUserProfile.fragments = {
  countries: CountrySelector.fragments.countries
}

Here I think it would be reasonable to fetch countries via RSC but the data needs to be used in an interactive component. Passing fetched data to a Client Component is where RSC works really great IMO, the problem really arises when static properties like GraphQL fragments need to be read unfortunately.

  • Even then, most component files will probably not marked "use client", but only the "boundary files". So apart from that boundary file, it should be okay to import that

Interesting point. That's true, however as soon as you add interactive React features to a "shared" component like useEffect, you can't import it from an RSC anymore:

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

(continuing from the Twitter conversation) In addition to what I already said in the issue, I think the top problem is that multiple different bundlers would need to add support for that, which will probably not happen.

That might be true, yes. I was kind of hoping that this pattern could be considered at this stage, as RSC bundlers are being created currently and Next.js has the only one that is known to be stable AFAIK.

phryneas commented 10 months ago

My first intuition was to just export const fragments from your component rather than adding those as a static member, but I'm not sure if you run into the same problem.

Unfortunately, that causes the same problem!

Everything imported from a client boundary is available as [Function (anonymous)] in the importing module. Calling the function results in:

  • Even then, most component files will probably not marked "use client", but only the "boundary files". So apart from that boundary file, it should be okay to import that

Interesting point. That's true, however as soon as you add interactive React features to a "shared" component like useEffect, you can't import it from an RSC anymore:

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

Yeah, I fear that might not be the best angle going forward - the React team is pretty set on having that a "file-level" switch, and I can understand it as everything else will make things a lot more complicated.
As much as I like the Component.fragments pattern, maybe we will need different patterns. Maybe the "near-operation-file" preset will just be the better choice going into the future?
So you'd have a MyComponent.tsx, a MyComponent.graphql and a (generated) MyComponent.graphql.ts - and you'd import the fragments from MyComponent.graphql (with ts automatically adding the .ts file extension).

Hmm, I read this part in the docs. I'm wondering from which perspective this is desirable? Is this seen as a necessity to work within the technical constraints we have or does it solve a problem for users or developers?

Here's a practical example for a component that can be used for updating a user profile:

[...]

Here I think it would be reasonable to fetch countries via RSC but the data needs to be used in an interactive component. Passing fetched data to a Client Component is where RSC works really great IMO, the problem really arises when static properties like GraphQL fragments need to be read unfortunately.

Yeah, this would be the rare example where it makes sense - but also I have to admit that this is a pattern I still have to wrap my head around a bit.
My assumption would have been that you would always use the cache to access from the component, not pass in data using props - which would make the "server vs client" divide a lot more apparent. I think querying data on the server and passing it in as props would be a good signal to the dev that this data should be considered static, so I wouldn't generally recommend against that usage of data - but at the same time, passing user in as a prop here seems like it should be client-side data, so not passed in as a prop, but rather read using the useFragment hook.

phryneas commented 9 months ago

My first intuition was to just export const fragments from your component rather than adding those as a static member, but I'm not sure if you run into the same problem.

Unfortunately, that causes the same problem!

@amannn I just had an alternative thought - you wouldn't even need to move the fragment out of the Client Component file if you used the "near-operation-file" preset for graphql-codegen. You would import the fragment not from ./ClientComponent, but from ./ClientComponent.generated, and that file would only contain the compiled fragments, not the Client Component itself, so the bundler should be fine!

phryneas commented 1 month ago

I'm doing some housekeeping so I'm closing some older issues that haven't seen activity in a while. If this is still relevant, please feel free to reopen the issue.

github-actions[bot] commented 1 month ago

Do you have any feedback for the maintainers? Please tell us by taking a one-minute survey. Your responses will help us understand Apollo Client usage and allow us to serve you better.