-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Rework fragment docs #12155
Rework fragment docs #12155
Conversation
✅ Docs Preview ReadyNo new or changed pages found. |
|
commit: |
size-limit report 📦
|
✅ Deploy Preview for apollo-client-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for apollo-client-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
@@ -45,15 +45,15 @@ If we later _change_ which fields are included in the `NameParts` fragment, we a | |||
|
|||
## Example usage |
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.
I made these changes here to try and move away from the notion that fragments should only be used as a DRY tool and instead should be combined with components. While it can be used for DRY, the composition unit should be the component itself rather than the fragment.
f208671
to
4506df8
Compare
docs/source/data/fragments.mdx
Outdated
``` | ||
|
||
After you define a fragment in a child component, the _parent_ component can refer to it in its _own_ colocated fragments, like so: | ||
|
||
```js title="FeedEntry.jsx" | ||
FeedEntry.fragments = { |
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.
I'm making a bold choice and moving away from this .fragments
pattern that we've had in our docs for so long for a couple reasons:
- Exports are fairly typical
- Some React component TypeScript types (i.e.
React.FC
orforwardRef
) complain when you try and add static properties on them. This avoids that issue. - Data masking will require these fragments to be used with
useFragment
. Its much more natural to use a variable than the static property:
useFragment({
fragment: FeedEntryFragment
})
// vs
useFragment({
fragment: FeedEntry.fragments.entry,
})
import { gql } from '@apollo/client'; | ||
|
||
export const CORE_COMMENT_FIELDS = gql` | ||
fragment CoreCommentFields on Comment { | ||
export const COMMENT_FRAGMENT = gql` |
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.
I'm noticing some of our examples use screaming snake case for variables that are assigned fragments, but others use PascalCase. Should we standardize in our docs?
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.
+1 for being consistent with style, no opinion on which one is better
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.
+1 on agreeing on a convention and not having a preference.
LMK what you decide on and I can add it to our style guide!
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.
I went ahead and settled for SCREAMING_SNAKE_CASE in this doc, specifically to avoid a conflicting name with a TypeScript type of the same name.
For example,
import type { CommentFragment } from './path/to/gql/types.ts';
// Oops we have a conflicting name with the imported type!
const CommentFragment = gql`
fragment CommentFragment on Comment {
# ...
}
`
This allows us to reuse the "comment fragment" name without the conflict and avoids the need to get creative either by aliasing the import or coming up with a new convention on the variable name itself.
Hope this clarifies it!
docs/source/data/fragments.mdx
Outdated
|
||
Starting in Apollo Client 3.7, fragments can be registered with your `InMemoryCache` so that they can be referred to by name in any query or `InMemoryCache` operation (such as `cache.readFragment`, `cache.readQuery` and `cache.watch`) without needing to interpolate their declarations. | ||
Starting in Apollo Client 3.7, fragments can be registered with your `InMemoryCache` instance so that they can be referred to by name in any query or `InMemoryCache` operation (such as `cache.readFragment`, `cache.readQuery` and `cache.watch`) without needing to interpolate their declarations. |
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.
Since I've added the MinVersion
tag, do we need to keep "Starting in Apollo Client 3.7" or should I get rid of it?
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.
I think we can get rid of it 🔪
docs/source/data/fragments.mdx
Outdated
|
||
Consider the following view hierarchy for an app: | ||
|
||
``` | ||
FeedPage | ||
└── Feed |
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.
I got rid of this intermediate Feed
component in the example because its not mentioned anywhere and I wanted to add the note about fragments only being used by their parent.
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.
LGTM - added a few thoughts and a suggestion!
import { gql } from '@apollo/client'; | ||
|
||
export const CORE_COMMENT_FIELDS = gql` | ||
fragment CoreCommentFields on Comment { | ||
export const COMMENT_FRAGMENT = gql` |
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.
+1 for being consistent with style, no opinion on which one is better
docs/source/data/fragments.mdx
Outdated
|
||
Starting in Apollo Client 3.7, fragments can be registered with your `InMemoryCache` so that they can be referred to by name in any query or `InMemoryCache` operation (such as `cache.readFragment`, `cache.readQuery` and `cache.watch`) without needing to interpolate their declarations. | ||
Starting in Apollo Client 3.7, fragments can be registered with your `InMemoryCache` instance so that they can be referred to by name in any query or `InMemoryCache` operation (such as `cache.readFragment`, `cache.readQuery` and `cache.watch`) without needing to interpolate their declarations. |
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.
I think we can get rid of it 🔪
cc777a5
to
b36dd77
Compare
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.
Just some suggestions for clarity! I dig bumping up the colocation bit (which was the part I focused on when reviewing.)
Tomorrow I'll make sure that the mermaid diagram can render!
import { gql } from '@apollo/client'; | ||
|
||
export const CORE_COMMENT_FIELDS = gql` | ||
fragment CoreCommentFields on Comment { | ||
export const COMMENT_FRAGMENT = gql` |
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.
+1 on agreeing on a convention and not having a preference.
LMK what you decide on and I can add it to our style guide!
Co-authored-by: Maria Elisabeth Schreiber <[email protected]>
Co-authored-by: Maria Elisabeth Schreiber <[email protected]>
Co-authored-by: Maria Elisabeth Schreiber <[email protected]>
This PR does not yet contain docs on data masking, however this does make some tweaks to the existing fragments doc to make fragment colocation more prevalent and recommended. This should setup the data masking sections nicely.