Skip to content
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

Consider id field addition dangerous #42

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

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

@@ -843,7 +843,11 @@ class FieldAdded < AbstractChange
attr_reader :object_type, :field, :criticality

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