Skip to content

Add document with suggestions for ID fields #395

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Jul 14, 2020
Merged

Add document with suggestions for ID fields #395

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b09002b
Add document with suggestions for ID fields
DrewML Jul 8, 2020
9ecb46b
Added note about marking ID field as non-nullable
DrewML Jul 9, 2020
48cfec5
always with the typos
DrewML Jul 9, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions design-documents/graph-ql/id-fields-object-types.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
# ID Fields in Object Types

When a GraphQL Object Type is used to represent an entity that can be referenced by ID, it _should_ follow these best-practices.

## Best Practices

### Do

- Use the `ID` scalar type
- Make the `ID` field non-nullable (syntax: `ID!`)
- Use the field name `id` or `_id`
- Most GraphQL client libs automatically use this field for caching. Any other field name will require manual caching logic on the client
- https://www.apollographql.com/docs/react/caching/cache-configuration/#assigning-unique-identifiers
- https://formidable.com/open-source/urql/docs/graphcache/normalized-caching/#key-generation
- Include an ID field anytime an Object Type _could_ have an ID field

### Do Not
- Include info in the client-facing description describing how the field is encoded/decoded (should be opaque)
- Example: The client shouldn't know if an `ID` is a base64-encoded integer
- Use `String` or `Int` type
- Use duplicate IDs across a concrete type. In other words, if the client wants to produce a cache key, the concatenation of a `__typename` + `id` field should _always_ be unique

Copy link
Contributor

@tjwiebell tjwiebell Jul 8, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It feels odd to include this, but SelectedConfigurableOption violates this rule, and causes odd behavior with automatic caching. This id field is based on ConfigurableProductOption.id, which is not unique across all responses. Doesn't hurt to be explicit, but consider this optional.

Suggested change
- Use an ID value you cannot guarantee is unique across all responses

Copy link
Contributor Author

@DrewML DrewML Jul 8, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This id field is based on ConfigurableProductOption.id, which is not unique across all responses.

Whoa. Just to make sure I'm reading this right, you're saying that I can get 2 different ConfigurableProductOption types from the API that collide on the id field, but hold different data in the other fields?

Copy link
Contributor

@tjwiebell tjwiebell Jul 8, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep! We ended up keying on value_id instead, which I didn't consider might also not be unique 😶

Screen Shot 2020-07-08 at 4 31 47 PM

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

which I didn't consider might also not be unique

Oh noooooo. Well, you've definitely highlighted that we've got some work to do, but trending in the right direction!

Going to push an update to the document

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added this under Do Not, let me know if you have suggestions to tweak the wording:

Use duplicate IDs across a concrete type. In other words, if the client wants to produce a cache key, the concetenation of a __typename + id field should always be unique.

## Examples where an ID is not helpful

- Wrapper types, like `SearchResultPageInfo`