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

Make storing IDs in arrays consistent with the rest of the store #901

Merged
merged 8 commits into from
Nov 15, 2016

Conversation

stubailo
Copy link
Contributor

@stubailo stubailo commented Nov 12, 2016

I was hacking around with some ideas for inspecting the store, and ran into a problem - ID references are not always stored the same way.

When it's an object field, the reference looks like:

fieldKey: {
  type: 'id',
  id: 'asdf',
  generated: false,
}

When it's an array field, it's not wrapped at all:

fieldKey: [
  'asdf',
  'qwer',
]

This is problematic if one wants to write a tool that can follow query trees through the store. Looks like when we initially refactored to escape these IDs in the store, we didn't go all the way and totally forgot about arrays.

This PR fixes it to look like:

fieldKey: [
  { type: 'id', id: 'asdf', generated: false },
  { type: 'id', id: 'qwer', generated: false },
}

Note: This involved updating some of the old mutation behavior code. We should deprecate that and delete it/move it to an auxiliary package.

Questions:

  • Is this a breaking change? I suppose in some ways it is, because it changes the store format. But I would be surprised if people were relying on the old behavior because someone else would have brought up that it was inconsistent?

TODO:

  • Make sure all of the significant new logic is covered by tests
  • Rebase your changes on master so that they can be merged easily
  • Make sure all tests and linter rules pass
  • Update CHANGELOG.md with your change

@helfer
Copy link
Contributor

helfer commented Nov 14, 2016

@stubailo lgtm, shall I merge this before the next release?

@stubailo
Copy link
Contributor Author

Yes, can you update the CHANGELOG as well? I forgot to do it and I'm not near a terminal at the moment.

@helfer helfer merged commit 6de063c into master Nov 15, 2016
@helfer helfer deleted the array-id-value branch November 15, 2016 07:53
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 18, 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.

2 participants