-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Update description of Fragments to emphasize evolving data needs #1193
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
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for graphql-spec-draft ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Definitely agree on the motivation for this change; thanks for the submission Janette! |
I think that is is great change overall. The challenge is a bit to make it understandable in the context of the spec. Using fragments to declare data needs (like in relay) always requires something on top of the spec to make it work. So we would have to make this clear somehow. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple comments but otherwise very supportive of updating that language!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calling the logic friendProfile
and the fragment friendProfile
is confusing. Can we differentiate the logic - e.g. just call it "profile rendering logic" (prose) rather than using code? (Not sure if this is the best fix.)
duplicated text in the document. Inline Fragments can be used directly within a | ||
selection to condition upon a type condition when querying against an interface | ||
or union. | ||
Fragments allow for the definition of modular selection sets that make it easier |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to flush out what modular means. Matt + Pascal's suggestion: single purpose.
Fragments allow for the definition of modular selection sets that make it easier | ||
to add and remove selections as needed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe something along the lines of (written whilst on Zoom, not final wording!):
Fragments allow client components and logic to describe their specific
data requirements locally. Fragments can then be composed together to
form a GraphQL operation to issue to the server.
Why these changes?
Fragments are not for reuse.
The current language in section 2.8 on Fragments, namely the sentence...
...encourages using fragments to deduplicate common selections, even if those selections represent independent data needs.
For example, let's consider these two functions
Given these two functions currently both use
authorName
andcontentText
, the current language in the spec encourages one to write a fragmentIf
formatPostForFeed
now needstimestamp
, we will add naturally addtimestamp
toPostDisplayFragment
If we have the following queries
notice how
NotificationQuery
is now over-fetching timestamp!The key observation is that
formatPostForFeed
andformatPostForNotification
are two independent functions, so by having them both rely on a single fragment to express their data needs, we are creating a dependency where one should not exist (because that dependency does not exist in the product logic itself).What are the proposed changes?
Updated:
The goal is to emphasize that fragments support evolving data needs (as opposed to recommending people identify common selections that are currently in an executable document).
Open to any and all feedback on the motivation for the change and how it's communicated via changes in the spec language!