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

Mutation results #320

Merged
merged 36 commits into from
Jun 29, 2016
Merged

Mutation results #320

merged 36 commits into from
Jun 29, 2016

Conversation

stubailo
Copy link
Contributor

@stubailo stubailo commented Jun 27, 2016

See #317 for design, please give feedback.

TODO:

  • Make sure you don't have to manually type the dataId for store path
  • Update CHANGELOG.md with your change
  • 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

  • Review with @Poincare
  • Refactor mutation reducer arguments after talking to Abhi

Additional tests needed:

  • generateMutationResultDataId in ARRAY_INSERT
  • array reference equality after cleanArray, both on nested and non-nested arrays
  • DELETE on nested array
  • scope query with colliding fields: regular, inline fragment, and named fragment

@abhiaiyer91 abhiaiyer91 mentioned this pull request Jun 28, 2016
4 tasks
@stubailo stubailo force-pushed the mutation-results branch 3 times, most recently from 9c9dd48 to 37fa7eb Compare June 28, 2016 06:10
}

export type MutationArrayDeleteAction = {
type: 'ARRAY_DELETE';
Copy link
Contributor

Choose a reason for hiding this comment

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

we should prefix with APOLLO_ to be consistent no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These aren't real actions - they end up in the applyResults section of the MUTATION_RESULT action, here: https://github.com/apollostack/apollo-client/pull/320/files#diff-162241b86351f640a8bb3127ed5fbd5bR92

This is because we want to process all of the mutation result stuff synchronously in one pass, rather than as multiple actions one after the other - it seems weird to me to fire many actions in one tick, when they represent one event (a mutation arriving from the server).

Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to not call these actions at all? I was confused at first by this as well. It seems that these are more like "action applications" although I don't think that is a good name for them. I just think calling them "actions" makes it seem as though they are actual actions which will lead to a transition within the Redux store which can be a bit confusing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I'm going to call them "behaviors" after thinking about it.

@Poincare
Copy link
Contributor

I made a couple of comments but looks good to me overall. Excited for this to get merged!

@stubailo
Copy link
Contributor Author

A good day's worth of work, finally being merged! Feels good. And with good test coverage to boot. Now to write the docs...

@stubailo stubailo merged commit c9ea5bc into master Jun 29, 2016
@zol zol removed the in progress label Jun 29, 2016
@stubailo stubailo changed the title [WIP] Mutation results Mutation results Jun 29, 2016
@stubailo stubailo mentioned this pull request Jun 29, 2016
@stubailo stubailo deleted the mutation-results branch September 20, 2016 03:43
LevanArabuli pushed a commit to LevanArabuli/apollo-client that referenced this pull request Nov 3, 2022
@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.

4 participants