Skip to content

Conversation

@sambostock
Copy link
Contributor

While most of the time adding a field is a safe non-breaking change, there are certain fields that can be dangerous to add, namely the id field.

This is because many caching implementations will use the id field as part of a cache key, if present. For example, Apollo's InMemoryCache uses __typename and id (or _id) to generate a cache key.1

In fact, an ESLint-Plugin-GraphQL rule exists to enforce the querying of so called "required fields"2 (such as id), to ensure caching is done properly.

As such, adding an id field to an existing type can change the client side caching behaviour if the client hasn't defined a custom caching strategy (e.g. start caching resources that were previously not cached).

Footnotes

  1. https://www.apollographql.com/docs/react/v2/caching/cache-configuration/#default-identifiers

  2. https://github.com/apollographql/eslint-plugin-graphql#required-fields-validation-rule


def initialize(object_type, field)
@criticality = Changes::Criticality.non_breaking
@criticality = if field.graphql_name == 'id'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perhaps it should be possible to customize this?

For instance,

  • Apollo's InMemoryCache looks for id or _id by default
  • ESLint-Plugin-GraphQL's required-fields rule is customizable and uses uuid in its example

Copy link
Collaborator

Choose a reason for hiding this comment

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

🤔 I think customizing this might go into "linter" territory? Especially beyond the initial id only proposal.

Anyone consuming this library is free to do whatever they want with the output of course.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants