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

Added a mutations tab to track mutations #36

Merged
merged 1 commit into from
Jun 20, 2017

Conversation

ryannealmes
Copy link
Contributor

I thought it would be cool to track mutations via the devtools so I copied the queries section and modified it to deal with mutations.

You can find the discussion about this here #35

Note this is dependent on a change to the client. I will be pushing up the PR for that now. The specific change is to expose getMutationDefinition so that I can retrieve the definition of the mutation from the AST.

@apollo-cla
Copy link

@ryannealmes: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/

@rrdelaney
Copy link
Contributor

Thanks so much for implementing this!

What does the error handling do when the client isn't updated to have the change required? The dev tools should support most versions of the Apollo client, and if a certain version of the client doesn't support this feature, it would be nice if we could replace the pane with an error message saying "This version of Apollo Client doesn't support this feature", or something similar.

@ryannealmes
Copy link
Contributor Author

You can find the PR for the apollo-client here apollographql/apollo-client#1699

This functionality is dependent on that being pushed.

@ryannealmes
Copy link
Contributor Author

Right now the mutation will still display, but no error will be displayed. Currently on the query side of things you get an exclamation mark next to the query. I will look at the version error handling.

I would like to update the apollo-client so that mutations include graphQLErrors so that this functionality works. I first thought I should get this base functionality working so I don't need to push a huge PR.

One minor concern is that there is a lot of duplication between the queries and mutations tab. I could refactor into a single component, but I thought we should wait? It may be premature optimisation.

If someone could check my work please. I don't want bugs getting merged into apollo-devtools :)

@ryannealmes
Copy link
Contributor Author

I have looked at the code and it seems like the client doesn't return any thing when there is an error on the server side. This means that this functionality will sit in a loading state since the props/state is not updated with new data.

I am not really sure of a fix. I could put delay to check if query is still loading after 5 seconds and then display an error, but this feels wrong. The functionality feels fine as it is. @rrdelaney any suggestions welcome.

@rrdelaney
Copy link
Contributor

Sorry, poor wording on my part, although error handling from the server is still a good idea.

This change requires new functionality in Apollo Client. However, the devtools need to support older versions of the client than the one with your update. The Mutations pane needs to have error checking for the case where the Apollo Client version does not support getMutationDefinition.

@ryannealmes
Copy link
Contributor Author

Ah cool that makes sense. I will sort out the error handling and push a change.

@ryannealmes
Copy link
Contributor Author

ryannealmes commented May 14, 2017

I have added a check for getMutationDefinition. If it doesn't exist the tab will not be rendered. Let me know if this is good enough.

If you could:

@rrdelaney
Copy link
Contributor

Thanks for adding the check 😄

I'll test this later today and let you know if I find any bugs!

@tnrich
Copy link
Contributor

tnrich commented May 18, 2017

Hooray for this PR! Can't wait to have this functionality!

@ryannealmes
Copy link
Contributor Author

I have been playing around with it all week. Working like a charm :)

@rrdelaney
Copy link
Contributor

Now that the depending Client PR is in I'll test this one last time then merge 😃

@rrdelaney
Copy link
Contributor

Just an update: this looks good to go, but I'll merge it after a new version of the client is released. That way I can bump the version number of the client needed for building

@stubailo
Copy link
Contributor

Does anyone have a screenshot of what this looks like?

@ryannealmes
Copy link
Contributor Author

Something I would like to add is the output of the mutation under mutation string. But I thought that could be another PR.
screen shot 2017-05-24 at 6 53 59 am
screen shot 2017-05-24 at 6 54 08 am

@stubailo
Copy link
Contributor

Sorry - I just saw this recently, a few questions:

  1. What does "watched mutations" mean in this context? A mutation happens just once, so it's not really like an ongoing thing.
  2. Where are the variables coming from? The variables are usually passed when the mutation is called.

If the same mutation is executed multiple times, does it get shown twice in the list? In that case, is it more of a mutation log?

@ryannealmes
Copy link
Contributor Author

After looking at the library a lot more on another issue today, I see what you mean by what is a watched mutation. I guess it would make more sense if it just said Mutations. If the mutation is run multiple times it would display a second mutation in the devtools. I can rename this if you want?

The variables are passed into the mutate function. For example

const mutationConfig = {
  props: ({ mutate }) => ({
    updatePerson: (personCode, person) => {
      let variables = objectAssign({}, { personCode, person });
      return mutate({ variables });
    },
  })
};
...
export default compose(
  connect(mapStateToProps, mapDispatchToProps),
  graphql(query, config),
  graphql(mutation, mutationConfig)
)(EditPersonalInformationContainer);

Were you expecting variables to flow down to the devtools in a different way?

A side note:

From a new comers and surface point of view, watched queries is pretty confusing (I would have just expected queries), but after looking at the codebase (apollo-client) I sort of understand that some queries are watched and some are just executed. I actually have another issue I just logged around non-watched queries not remaining in the devtools as I thought they are meant to appear in Watched Queries (which they actually do temporarily). It would be great to get you to weigh in on that and perhaps point me to any documentation so I can understand the thinking behind the architecture of the apollo-client more. I am trying to get through the source code, but it is quite heavy, so any help would be appreciated.

@rrdelaney
Copy link
Contributor

rrdelaney commented May 28, 2017

Apollo Client 1.3 landed which means this can be easily tested! I tested this pretty thorughly todat and this feature looks awesome!

Before merging:

  • Update your PR to use Apollo Client 1.3 (npm i -S apollo-client@1.3 should do it)

Suggestions:

  • I don't know if I love "Watched Mutations". "Previously Run Mutations" or something similar would be better I think
  • I would remove the "Run in GraphiQL" button. If someone accidentally clicks the button it runs a mutation again, possibly leading to unexpected results.

Otherwise, this looks good to me 😄

@stubailo
Copy link
Contributor

Yeah, there might be some confusion since we haven't really explained well that what Apollo is doing under the hood is watching queries on the store. This isn't how mutations work. For a query there is an explicit status of "active" or "not active" since they are a long-running operation on the client (think of it as a client-side livequery). But for mutations it's a one-off operation.

I think "mutation log" would be the best name, and I think having a "show in GraphiQL" option would be really nice but it shouldn't run the mutation right away. IMO being able to debug mutations would be really great.

@rrdelaney
Copy link
Contributor

"Show in GraphiQL" sounds awesome 👍

@ryannealmes
Copy link
Contributor Author

ryannealmes commented May 29, 2017

Cool I will try get this done in the next couple of days.

It would be rad to see more documentation on the internal architecture / under the hood of the apollo-client. I have been managing to piece together bits of it.

@tnrich
Copy link
Contributor

tnrich commented Jun 5, 2017

Any way this could be released as is and have the enhancements @stubailo mentioned merged in later? It would be great to be able to use this even if it isn't perfect yet. Just my 2 cents :)

@ryannealmes ryannealmes force-pushed the mutations-panel branch 3 times, most recently from 7987867 to 44f5aa5 Compare June 13, 2017 21:02
@ryannealmes
Copy link
Contributor Author

ryannealmes commented Jun 13, 2017

I have made all the requested changes, I am not exactly sure what to do about the index_bundle.js file. When I included it, there were conflicts. I am guessing this is something the maintainers will add when the do a new release of the devtools?

Everything works fine on my machine and has been for a while. I will continue to test locally with the new changes (if others can test too that would be great). I have also updated to the latest apollo-client so it should be much easier for guys to test.

Sorry for taking so long to get to this, but I am pretty busy with work at the moment. K bed time 💤

@rrdelaney
Copy link
Contributor

Thanks for making the changes! I'll review one last time, and then looks good to go!

I'll take care of the compiled file when releasing 🙂

@rrdelaney
Copy link
Contributor

This looks great 😄 It works super well for me with no issues, and "Show in GraphiQL" is awesome

@rrdelaney rrdelaney merged commit a1234fa into apollographql:master Jun 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants