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

Static fragments on Client Components #27

Closed
amannn opened this issue May 24, 2023 · 7 comments
Closed

Static fragments on Client Components #27

amannn opened this issue May 24, 2023 · 7 comments

Comments

@amannn
Copy link

amannn commented May 24, 2023

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: amannn@fa8c9d3

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
Copy link
Member

phryneas commented May 24, 2023

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.

  • 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:
    image
  • The only point where this would be unfortunate would be with the idea of a PrefetchQuery component that could be rendered in SSR , but only to transport that data into SSR/the client.
  • 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
  • I would suggest to change that example in your PR to <User id={data.getUser.id} /> and call getClient().readFragment from there - that would be analogous to our use of useSuspenseQuery and useFragment.

@jerelmiller
Copy link
Member

@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
Copy link
Author

amannn commented May 25, 2023

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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.

Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants