Ideas for improving mutations, mutation results, and optimistic UI

[stubailo]

Right now, we have a working implementation of mutation results and optimistic UI, which can be seen in this PR to GitHunt: https://github.com/apollostack/GitHunt/pull/64/files

First, let's clear up the difference between mutation result handling and optimistic UI for that:

1. Mutation results are achieved via resultBehaviors on the mutation. These tell the client where to put the result into the store, once a result is received. You need this if you want to avoid refetching queries after the mutation result, even if you don't want optimistic UI.
2. Optimistic UI is achieved by specifying a fake response from the server, which is then passed through (1) to apply a patch to the optimistic part of the store. This is rolled back once the mutation returns, and replaced with the real result, which is applied in the same way.

Issues with mutations

• Right now, you need to declare the variables in three places because of the operation definition
• You need to write some sort of "mutation name"/"mutation field" five times if you are using mutation results and optimistic UI. This can be seen in the PR if you count the number of times we write "submitComment".

Issues with mutation behaviors

• You need to make sure that the fields in your mutation result match up with the fields you need to render in the UI, which basically means it should match the fields you ask for in the queries on this page of the UI. This can be mitigated using fragments, but that makes optimistic UI harder, see below.
• Some result behaviors involve using the storePath to locate data. This isn't well documented and might require a lot of knowledge of internals.
• If you are generating a store path, you need to guess the dataId of the object you want. It's hard to access the dataIdFromObject function in react-apollo.

Issues with optimistic UI

• If you are using a query transformer, you will need to include fields like __typename in the optimistic result. This is unfortunate, because this is something you usually don't have to worry about.
• Some data is generated by the server. For example, IDs are often sequential SQL IDs, and you can't predict what they will be. Moreover, you don't want these to collide with existing data on the client, so you might need to be careful with which values you pick.
• If you use fragments in the mutation query, then your optimistic response needs to include the data returned by the fragment. If the fragment is defined somewhere far away in the code, you might need to jump around to figure it out. Plus, if you later edit the fragment, you might need to update a lot of optimistic responses (which might not be impossible since you can grep for the name of the fragment, but that might not always work, or it could be too much effort if the fragment is used everywhere)

Ideas

• Generate the mutation definition and argument definitions by using the schema or the variables at runtime
• Use the shape of the result query to generate the right shape of result but with nulls

To be continued....

Properties we want to preserve

• Static analysis of gql queries
• Ability to copy-paste gql queries to graphiql (related to the first item)
• Avoid fetching/involving schema
• Compatibility with any graphql server (no need for a special configuration)

Properties nice to preserve

• Ability to call multiple mutations at once

[dahjelle]

I don't mean to add too many thoughts into this discussion, but I wonder if reactivity is also related? I've been adding updates to the Apollo store from a CouchDB _changes feed, and I just recently used one of the mutation behaviors (mutationResultDeleteReducer) as part of it.

Some of the discussion on reactivity that I've seen:

#180: updating the apollo store after creation
#172: write up possible reactivity designs

[Slava]

@dahjelle seems like what you are describing is not relevant to this issue. That being said, it is something we are interested in supporting too. Maybe open a separate issue for a discussion? Thank you.

[Slava]

A new idea how to do it with a reducer-style function:

    submitComment: (repoFullName, repoId, commentContent, currentUser) => ({
      mutation: gql`
        mutation ($repoFullName: String!, $commentContent: String!) {
          submitComment(repoFullName: $repoFullName, commentContent: $commentContent) {
            postedBy {
              login
              html_url
            }
            createdAt
            content
          }
        }
      `,
      variables: {
        repoFullName,
        commentContent,
      },
      optimisticResponse: {
        submitComment: {
          postedBy: currentUser,
          content: commentContent,
        },
      },
      updateQueries: {
        Comment: (prevRes, newComment, vars) => {
          const prevEntry = prevRes.entry;
          return {
            Entry: {
              ...prevEntry,
              comments: [newComment, ...prevEntry.comments],
            }
          };
        }
      },
    }),

as opposed to:

  mapQueriesToProps: ({ ownProps }) => ({
    data: {
      query: gql`
        query Comment($repoName: String!) {
          # Eventually move this into a no fetch query right on the entry
          # since we literally just need this info to determine whether to
          # show upvote/downvote buttons
          currentUser {
            login
            html_url
          }
          entry(repoFullName: $repoName) {
            id
            postedBy {
              login
              html_url
            }
            createdAt
            comments {
              postedBy {
                login
                html_url
              }
              createdAt
              content
            }
            repository {
              full_name
              html_url
              description
              open_issues_count
              stargazers_count
            }
          }
        }
      `,
      variables: {
        repoName: `${ownProps.params.org}/${ownProps.params.repoName}`,
      },
    },
  }),
  mapMutationsToProps: () => ({
    submitComment: (repoFullName, repoId, commentContent, currentUser) => ({
      mutation: gql`
        mutation submitComment($repoFullName: String!, $commentContent: String!) {
          submitComment(repoFullName: $repoFullName, commentContent: $commentContent) {
            postedBy {
              login
              html_url
            }
            createdAt
            content
          }
        }
      `,
      variables: {
        repoFullName,
        commentContent,
      },
      optimisticResponse: {
        __typename: 'Mutation',
        submitComment: {
          __typename: 'Comment',
          postedBy: currentUser,
          createdAt: +new Date,
          content: commentContent,
        },
      },
      resultBehaviors: [
        {
          type: 'ARRAY_INSERT',
          resultPath: ['submitComment'],
          storePath: [`Entry ${repoId}`, 'comments'],
          where: 'PREPEND',
        },
      ],
    }),

[saikat]

For 'You need to make sure that the fields in your mutation result match up with the fields you need to render in the UI, which basically means it should match the fields you ask for in the queries on this page of the UI. This can be mitigated using fragments, but that makes optimistic UI harder, see below' -- am I right in thinking that this also means that if you have any components outside of the current component that relies on data affected by the mutation, the result from the mutation will need to make sure to fetch the properties for those other components? e.g.:

ComponentA fetches:

todo {
  id
  somePropertyDerivedFromTitleThatChangesWhenTitleChanges
}

ComponentB fetches:

todo {
  id
  title
}

Now if ComponentB has a mutation that edits the title, will the result of the mutation need to return somePropertyDerivedFromTitleThatChangesWhenTitleChanges so that ComponentA gets correctly updated?

[stubailo]

@saikat Yes, that's correct. But I'm not sure how to get around this. I guess one option is to somehow keep track of which fields are accessed on that todo item so that they can all be refetched.

I feel like for complex cases (when lots of parts of the UI are being updated) it might be pretty reasonable to refetch queries, but for simple cases the current approaches will work?

[saikat]

I wonder if having an option to just fetch every field on the mutation
result would be better than having each of the dependent queries be
invalidated and do a refetch. This doesn't solve the list insertion
problem, but if that happened out of the box, I would personally probably
just move to a model where all my lists are owned by a 'collection' type
where the collections have unique IDs so I don't have to worry about
dealing with updating my lists.

I do think that the complex case may be the more common one to handle as a
lot of the value of a framework like Apollo is the cross-component data
management bit of a SPA (and I love frameworks like Apollo that make sure
you don't accidentally shoot yourself in the foot up front just for the
sake of giving you an easy way to get started). For simple SPAs where it is
known that the app won't ever have a lot of components, it's easier to get
a data-backed app running using normal ajax with a rest endpoint and
something like superagent.

On Tue, Jul 26, 2016 at 3:40 AM Sashko Stubailo notifications@github.com
wrote:

@saikat https://github.com/saikat Yes, that's correct. But I'm not sure
how to get around this. I guess one option is to somehow keep track of
which fields are accessed on that todo item so that they can all be
refetched.

I feel like for complex cases (when lots of parts of the UI are being
updated) it might be pretty reasonable to refetch queries, but for simple
cases the current approaches will work?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#398 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAFHRh9Gn_0yo9qh4-dBo0T8yWjxix4Hks5qZbnQgaJpZM4JLw-P
.

[amannn]

I've read in your new Medium article on mutations about the new updateQueries features.

Is this really the best way to do this? I know that the underlying problem is somewhat difficult, but this still feels somewhat hack-ish to me. Doing optimistic updates is one thing, but they will be corrected very soon by a server response. As far as I understand the result of updateQueries is now the definitive new list of todos in the store (at least as long as the todos aren’t fetched again), right?

I can think of the following problems:

• While this works for simple sorting orders like creation date, the client might not be able to do this for other sort criteria like sorting by popularity or simpler things like the author of a todo, where the server maybe takes both the firstname and the lastname of the author into account, the client could insert the new todo based on a sorting only by firstname, if it doesn't know know about that. In general, duplicating this kind of logic might be somewhat fragile.
• Paging becomes more difficult. While this could work for cursor-based approaches, an offset-based strategy could become out of sync.

I've read in the documentation about Designing mutation results that it's good practice to fetch the parent of the mutated item. Couldn't this also apply to this scenario? The developer could tell the apollo client which lists the item could be part of, and apollo would re-fetch at least the ids of the items in the lists (or parts, if paginated).

I might be totally wrong though, I'm not experienced with GraphQL and Apollo yet 🙂. In any case, it's definitely good that there is a feature for dealing with updating lists based on mutations.

[stubailo]

@saikat @amannn Note - if you fetch the parent of the mutated item or array, and you have IDs set up for your objects, it "just works" without using updateQueries: http://docs.apollostack.com/apollo-client/mutations.html#new-object-or-updated-fields

a lot of the value of a framework like Apollo is the cross-component data management

There's another aspect of updateQueries that isn't very clear from our blog post/documentation - it doesn't just update the result of that one query, the update is actually saved back to the store in a normalized way. So if other queries in your UI are looking at the same data, they are automatically updated as well.

It feels like we added a feature that is almost too simple to use - it looks like it doesn't really do much, when in fact it does a lot of great things underneath that you just don't have to think about when using it. Perhaps just documenting this better and explaining how to use it in several cases would be enough?

In short, updateQueries doesn't need to operate on every query - you can just update one and all of the rest will happen automatically because of store normalization.

While this works for simple sorting orders like creation date, the client might not be able to do this for other sort criteria like sorting by popularity or simpler things like the author of a todo

I don't think updateQueries should be the only thing people use. In my mind there are three main ways people can handle mutations:

1. Mutation results are normalized to the store by default - you don't have to do anything to make this happen other than define dataIdFromObject
2. Complex cases can be handled simply by refetching the entire query, then you don't have to worry about data consistency or anything
3. Cases where you want to use the result of the mutation to do a fine-grained update can be done with updateQueries, in which case you might do it incorrectly but you don't have to refetch a lot of data

IMO (1) and (2) are great for many situations, but (3) is useful for the case where you know exactly what needs to be updated for a particular mutation, and don't want to refetch too much data.

Paging becomes more difficult. While this could work for cursor-based approaches, an offset-based strategy could become out of sync.

In complex pagination scenarios I think refetching the query will be the right approach.

[amannn]

Thanks for the detailed response!

"In short, updateQueries doesn't need to operate on every query - you can just update one and all of the rest will happen automatically because of store normalization."

You mean the regular normalization technique with dataIdFromObject, right? That should work regardless of if you implement updateQueries, doesn't it? Or is there something special when you use updateQueries?

Btw. my comment is mostly about mutations that create or delete entities – I think updates are covered really well with normalization.

Regarding mutation handling strategy #2 you mentioned: From your experience, do you know a good way how to do this in a way that the component that triggers the mutation doesn't have to know about which lists should be re-fetched because of that mutation?

In a somewhat bigger redux app I'm currently working on, I solve this in the action creators. It's easy to invoke a re-fetch of particular lists when a entity got created that could potentially be in that list. It's not perfect, but at least there's one central place that invokes the list refetches, instead of all components that can create such entities knowing about which lists could potentially update.

Does it make sense to do something like this in Apollo? Maybe call all mutations from a central place, where hooks for re-fetches can be registered? Or watch the actions that Apollo dispatches and if a particular mutation is successful, call some re-fetches? Whatever approach makes sense, I think this can be solved in user land – just wondering if you've experimented with something like this.

EDIT: Maybe realtime queries for lists will eventually be the most elegant way to solve this specific problem.

EDIT 2: Just found issue #448. That's actually exactly what I'd need for my use case. The only thing I'd consider is to set up invalidateQueries in a more central place, not colocated to components. Basically saying "If mutation XYZ ever happens across the app, make sure to reload these queries if they were fetched so far.". Maybe an improvement to that would be to somehow specify what should be re-fetched when a query gets invalidated. E.g. for some mutations it's enough to fetch the ids of the entities within a list, because it's just a matter of the right order and not completely fetching all entities again.

[Poincare]

Can we close this issue now that each of these have been implemented fully? @stubailo @Slava

[Slava]

yeah, I used this issue as a place to discuss ideas