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

Fix cache normalization for mixed fields #3422

Merged
merged 6 commits into from
May 24, 2018
Merged

Fix cache normalization for mixed fields #3422

merged 6 commits into from
May 24, 2018

Conversation

dferber90
Copy link
Contributor

@dferber90 dferber90 commented May 7, 2018

Summary

Given a query

query {
  animals {
    species {
      name
    }
  }
}

which gets executed twice where species could either be of GQL type Dog or Cat, and where the responses have the species types inlined with no dataId and occur sequentially, in this order:

animals: [{
  __typename: 'Animal',
  species: {
    __typename: 'Cat',
    name: 'cat',
  },
}],
animals: [{
  __typename: 'Animal',
  species: {
    __typename: 'Dog',
    name: 'dog',
  },
}],

the store should finally contain

{
  '$ROOT_QUERY.animals.0.species': { name: 'dog' },
  ROOT_QUERY: {
    animals: [{
      generated: true,
      id: 'ROOT_QUERY.animals.0',
      type: 'id',
      typename: 'Animal',
    }],
  },
  'ROOT_QUERY.animals.0': {
    species: {
      generated: true,
      id: '$ROOT_QUERY.animals.0.species',
      type: 'id',
      typename: 'Dog',
    },
  },
}

However, without this PR it would wrongly contain '$ROOT_QUERY.animals.0.species': undefined (and some other stuff).

Description

It seems that the cache attempted to do cleanup of a generated object in case the reference to it gets replaced with one where we're actually provided a non-generated id. However, this also ran when a generated object's type changed, thus accidentally removing the newly generated object.

Details are in #3419

closes #3419

@dferber90
Copy link
Contributor Author

🤕 This bug is still relevant and causing us issues in production

It would be helpful to get a rough idea on when somebody will get to review this, so that we can figure out whether it's worth to create a workaround in the meantime.

Thanks!

@dferber90
Copy link
Contributor Author

It has been two weeks :( Anyone?

@tdeekens
Copy link

This is now affecting us too.

@n1ru4l
Copy link
Contributor

n1ru4l commented May 22, 2018

@jbaxleyiii

@hwillson hwillson self-assigned this May 23, 2018
@hwillson
Copy link
Member

At a first quick glance this looks awesome @dferber90 - thanks very much for working on this! I've slotted this in for a more thorough review, and will have updates to you shortly. 🤞 this should be merged in sometime tomorrow (and will be included in next Tuesday's release).

Copy link
Member

@hwillson hwillson left a comment

Choose a reason for hiding this comment

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

This all looks great @dferber90 - thanks very much for your work on this. LGTM!

@hwillson hwillson merged commit 4b96be9 into apollographql:master May 24, 2018
@dferber90
Copy link
Contributor Author

🎈Excellent, thanks for taking the time! I'm looking forward to the release

@dferber90 dferber90 deleted the df-fix-cache-normalization-for-mixed-fields branch May 24, 2018 12:18
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants