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

useFragment_experimental feedback #10650

Closed
adamesque opened this issue Mar 13, 2023 · 4 comments
Closed

useFragment_experimental feedback #10650

adamesque opened this issue Mar 13, 2023 · 4 comments

Comments

@adamesque
Copy link

Hi folks, wanted to provide some feedback on Apollo Client (JS)'s useFragment_experimental hook after we've played with it a little.

  • We noticed that returnPartialData defaults to true but doesn’t wrap the return type in Partial<TData> which means our consuming code isn’t type-safe anymore. We could do this manually at call sites but it’s a potential footgun.
  • We’ve found the exception-throwing behavior when returnPartialData is false to be very unergonomic since wrapping a hook in try/catch violates the rule against conditional hook execution (at least as far as linters are concerned). Instead, useFragment should return { data: null, complete: false, missing: MissingTree } or a classic {data, loading, error} tuple like useQuery, potentially governed by an error policy (this might be overkill though). Speaking of which…
  • The current hook doesn’t surface a loading flag tracking whether the requested fragment is included in a query that is also currently loading. Ideally this would be paired with a useSuspenseFragment which would suspend while a parent query is loading / before data is available.
  • We’ve run into so many issues with subsequent queries clobbering non-normalized fragment fields and causing fragment cache misses that our in-house useFragment hook currently performs automatic query refetches via a query/fragment registration scheme. This isn’t necessarily something that should be part of a core fragment API, but we believe Relay’s cache may better handle these scenarios. Might not be solvable with a completely normalized cache, and we're exploring solutions to help ensure that
    • queries are linted to ensure that id or key fields are always selected
    • non-normalizable types have appropriate merge policies set

I was glad to see some community discussion around fragments surface this weekend, and like I've commented in the past, it would be great to have fragment-first alternatives to Relay out there in the ecosystem, so I'm excited to see where this is headed. Thanks for all your work on this feature!

@bwhitty
Copy link
Contributor

bwhitty commented Mar 14, 2023

I too would like to see a further discussion of around Apollo's architectural take on query composition via fragments which can be colocated on components.

Should Apollo ship a build-time tool like Relay?
Should composing queries from Fragments become the default documented approach? And if Apollo doesn't ship the tooling, could it document using something like graphql-code-gen?
Are there any other features / opinions / architectural decisions baked into Apollo Client today which does/does not support this approach today?

@alessbell
Copy link
Member

Hi @adamesque 👋

Thanks for taking the time to share this feedback! We've been discussing and considering these ideas, so we appreciate your patience here. I'll take each point individually:

  1. Good catch - @phryneas has opened a PR with the fix there that should go out in 3.7.13.
  2. We definitely agree that should not be throwing, we'll investigate further (and if you already have a reproduction we could make use of, that would be much appreciated.)
  3. Tracking the loading state of a fragment via useFragment has some challenges given the current API: since named fragments can be included in many different queries (made easier by the fragment registry introduced in 3.7), we don't (yet) have a way to tie the fragment back to an ObservableQuery. We believe this idea—connecting a fragment to a query—makes more sense in a future useSuspenseFragment, which is still in its design phase, and we'll share more about that as soon as we can.
  4. This issue of subsequent queries clobbering non-normalized fields causing, in your case, fragment cache misses is one we've seen before in different contexts. We agree there's probably more the cache can do when there's ambiguity in these cases, and while we don't have any concrete plans to share yet, feedback like yours is always helpful when prioritizing the ideas we're considering.

@nonreactive

One other feature particularly relevant to working with fragments I'd like to call attention to is the @nonreactive directive introduced with #10722. It will go out in the next 3.8 alpha release in a day or two, but there's a snapshot release in a comment on the PR if anyone wants to try it out sooner 🎉

The demo I'm running in the video is available here. A blog post is in the works but thought I'd share it in the meantime :)

CleanShot.2023-04-05.at.11.28.40.mp4

I'll close out this issue for now but it's tagged with the "discussion" label so it will never be locked. Thanks again 🙌

@swrobel
Copy link

swrobel commented Apr 29, 2024

I too would like to see a further discussion of around Apollo's architectural take on query composition via fragments which can be colocated on components.

Any idea if this discussion ended up happening anywhere? The docs seem unclear about whether this is possible with the current implementation of useFragment

@adamesque
Copy link
Author

You might be interested in #11666 which reveals some info about the team's intentions re: colocated fragments

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

No branches or pull requests

5 participants