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

Get rid of changedTodos in the outputFields of the MarkAllTodos mutation #126

Closed
mhart opened this issue Aug 20, 2015 · 7 comments · May be fixed by joseroubert08/relay#26, ebarahona/relay#34, mobilist/relay#19, enterstudio/relay-studio#28 or mobilist/relay#29

Comments

@mhart
Copy link
Contributor

mhart commented Aug 20, 2015

Specifically talking about this field:

changedTodos: {

I can't find a reference to it in the client code at all.

Is it just unused? If so, maybe it could have a comment (or be removed?)

@steveluscher
Copy link
Contributor

Thanks for pointing this out! It is unused in the current implementation, but the current implementation also refetches the todos, whether or not their complete flag has been flipped. This would also work:

  getFatQuery() {
    return Relay.QL`
      fragment on MarkAllTodosPayload {
        changedTodos {
          complete,
        },
        viewer {
          todos {
            completedCount,
          },
        },
      }
    `;
  }
  getConfigs() {
    return [{
      type: 'FIELDS_CHANGE',
      fieldIDs: {
        changedTodos: this.props.todos.edges[0].node.id,
        viewer: this.props.viewer.id,
      },
    }];
  }

…but having to specify at least one id for changedTodos in fieldIDs is awkward, especially because this mutation is designed to make supplying the actual todos optional.

Maybe there's something else that we could do here, like:

  /* ... */
  getConfigs() {
    return [{
      type: 'FIELDS_CHANGE',
      fieldIDs: {
        changedTodos: 'any',
        viewer: this.props.viewer.id,
      },
    }];
  }

… to signal that we want to intersect the fat query with the union of all the tracked queries for this type, and send the resultant query up with the mutation.

Thoughts, @yuzhi @joesavona, @yungsters?

@yungsters
Copy link
Contributor

I think the correct way to deal with this is to fetch completedCount on viewer { todos } as in your snippet. I think we can get rid of changedTodos because it never makes sense for a field on the client to be updated by it.

@steveluscher steveluscher changed the title Is changedTodos used in the MarkAllTodos mutation? Get rid of changedTodos in the outputFields of the MarkAllTodos mutation Sep 11, 2015
@steveluscher
Copy link
Contributor

…it never makes sense for a field on the client to be updated by it.

But we do need to update the complete field on each changed todo.

This has been implemented in relayjs/relay-examples#2

@mhart
Copy link
Contributor Author

mhart commented Jun 28, 2016

@steveluscher that repo doesn't seem to exist (or is private?)

@mhart
Copy link
Contributor Author

mhart commented Jun 28, 2016

@steveluscher
Copy link
Contributor

It's private, for a few moments longer. Stay tuned!

On Jun 27, 2016, at 5:12 PM, Michael Hart notifications@github.com wrote:

@steveluscher that repo doesn't seem to exist (or is private?)


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

@mhart
Copy link
Contributor Author

mhart commented Jun 28, 2016

Cool, I can see it now, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment