Add document with suggestions for ID fields #395
Conversation
DrewML
requested review from
tjwiebell,
kokoc,
sirugh,
nrkapoor,
paliarush,
cpartica,
danielrenaud and
prabhuram93
| - 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 | ||
|
|
There was a problem hiding this comment.
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.
| - Use an ID value you cannot guarantee is unique across all responses | |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Yep! We ended up keying on value_id instead, which I didn't consider might also not be unique 😶
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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+idfield should always be unique.
|
Added the following under
|
|
Never have we reached consensus this easily 😂 |
Rendered Document