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 component #1520

Merged
merged 19 commits into from Feb 16, 2018

Conversation

Projects
None yet
10 participants
@excitement-engineer
Copy link
Collaborator

excitement-engineer commented Jan 5, 2018

This is an implementation of the <Mutation /> component.

Note, this is still a WIP. I will be improving it over the next few days and adding tests.

@lifeiscontent

This comment has been minimized.

Copy link

lifeiscontent commented Jan 6, 2018

@excitement-engineer first thing I see is being able to pass variables to runMutation. If you can take variables from both the function and props it would make for a much more powerful API because then you can partially apply the function from props and pass the rest of the variables into the function itself.

error,
};

return children(this.runMutation, result);

This comment has been minimized.

@lifeiscontent

lifeiscontent Jan 6, 2018

What if we add props instead of result here? That way people can still use polling functions from the component as well.

@rosskevin
Copy link
Collaborator

rosskevin left a comment

Some WIP type feedback - I think we should go ahead and define our own MutationResult like we did with Query, as I'm not sure when apollographql/apollo-client#2795 will be handled.

client: PropTypes.object.isRequired,
};

constructor(props: any, context: any) {

This comment has been minimized.

@rosskevin

rosskevin Jan 6, 2018

Collaborator

props: MutationProps<TData, TVariables>

// update?: MutationUpdaterFn;
// notifyOnNetworkStatusChange?: boolean;

export interface MutationProps {

This comment has been minimized.

@rosskevin

rosskevin Jan 6, 2018

Collaborator

MutationProps<TData = any, TVariables = OperationVariables>


export interface MutationProps {
mutation: DocumentNode;
variables?: OperationVariables;

This comment has been minimized.

@rosskevin

rosskevin Jan 6, 2018

Collaborator

: TVariables

mutation: DocumentNode;
variables?: OperationVariables;
optimisticResponse?: Object;
children: (mutateFn: () => void, result?: any) => React.ReactNode;

This comment has been minimized.

@rosskevin

rosskevin Jan 6, 2018

Collaborator

This result is the from apollo-client mutate<T>(options: MutationOptions<T>): Promise<FetchResult<T>>

Note there is a typing issue on it so it cannot yet be fully typed results: apollographql/apollo-client#2795

Alternately, we could define our own MutationResult like we did in Query and solve the type problem ourselves.

});

try {
const response = await this.client.mutate({

This comment has been minimized.

@rosskevin

rosskevin Jan 6, 2018

Collaborator

This will ultimately need parameterized types but apollographql/apollo-client#2795 needs fixed first to use them.

excitement-engineer added some commits Jan 6, 2018

@lifeiscontent

This comment has been minimized.

Copy link

lifeiscontent commented Jan 20, 2018

@rosskevin @excitement-engineer what do you guys think about the following?

  1. use render as the prop for render props.
  2. use React.children to pass the props from Mutate into the children of Mutate.
  3. use this pattern for the Subscriptions and Query components as well.

I see this API being a lot more flexible, because 1. you get the render prop pattern, and 2. if you need to compose React Components together, it's really straight forward.

@rosskevin

This comment has been minimized.

Copy link
Collaborator

rosskevin commented Jan 20, 2018

  1. use render as the prop for render props.
  1. use React.children to pass the props from Mutate into the children of Mutate.
  2. use this pattern for the Subscriptions and Query components as well.

No, this will be far more confusing. I've already made my opinion known - children are for rendering. If render was used for all rendering in react, then yes, but that is not the case.

Along these lines, I have discussed with @excitement-engineer and I'm not even sure there is a great use case for a Mutation component at all. Given mutations aren't simply fired when they are rendered, and are usually fired in in a callback which will live in the class somewhere, I think the primary use case for mutation usage is with withApollo or ApolloConsumer to get the client and call client.mutate directly. I've done this with our entire codebase in conjunction with using the Query component.

@lifeiscontent

This comment has been minimized.

Copy link

lifeiscontent commented Jan 20, 2018

@rosskevin if I create a demo of what I discussed in a real use case would you consider otherwise? Or should I just create my own node module for the api I am thinking about?

The pattern I’ve discussed would compose incredibly well with apollo-link-state when providing local state used in turn as variables for a remote query/mutation

@rosskevin

This comment has been minimized.

Copy link
Collaborator

rosskevin commented Jan 20, 2018

@lifeiscontent I am not the ultimate authority on the decision, but my thoughts are centered around ability for many to understand. What is clear is that many would not comprehend passing parameters as children, therefore it would be a docs/education problem and would create more issues.

I suggest wrapping it as you desire. I have a wrapper on Query to tweak a few things to suit our opinions that aren't suitable for maney as well.

@kandros

This comment has been minimized.

Copy link
Contributor

kandros commented Jan 25, 2018

Might be worth to take a look at #1550 (comment)

Currently what is the state of this?

I would like to help

@excitement-engineer

This comment has been minimized.

Copy link
Collaborator Author

excitement-engineer commented Jan 25, 2018

@kandros I have been on holidays for the past few weeks. But I will be picking this PR up again in a couple of days when I get back:)

@lifeiscontent

This comment has been minimized.

Copy link

lifeiscontent commented Jan 26, 2018

@excitement-engineer when can I test the latest release? e.g. Mutate, Query, etc.

@kandros

This comment has been minimized.

Copy link
Contributor

kandros commented Jan 27, 2018

You can use Query by installing react-apollo@beta Mutation is WIP

@kandros

This comment has been minimized.

Copy link
Contributor

kandros commented Jan 27, 2018

@excitement-engineer totally personal feedback: I think onComplete andonError are against the purpose of using a function renderer, I find them redundant and not clear

@excitement-engineer

This comment has been minimized.

Copy link
Collaborator Author

excitement-engineer commented Jan 27, 2018

@kandros I think there is a use case for performing some custom operation as soon as the mutation is done or there is an error (e.g. updating the state or calling a callback in the props). This cannot be done using the render prop AFAIK. Therefore the onError and onComplete serve a different function from the render prop.

@kandros

This comment has been minimized.

Copy link
Contributor

kandros commented Jan 27, 2018

Since onComplete just pass data and onError pass the error, can’t we estimate the completion state by the loading prop and error state by state prop in the renderer? Am I missing some specific usecases?

@kandros

This comment has been minimized.

Copy link
Contributor

kandros commented Jan 27, 2018

I re read your answer, I get it now 👍🏻

excitement-engineer added some commits Jan 27, 2018

return;
}

this.verifyDocumentIsMutation(nextProps.mutation);

This comment has been minimized.

@leoasis

leoasis Jan 27, 2018

Collaborator

Do we want to do this only if the mutation is different from the previous one? It involves calling parser which I think does a couple things that may add unnecessary overhead if the mutation doesn't change, which should be most of the time

This comment has been minimized.

@excitement-engineer

excitement-engineer Jan 31, 2018

Author Collaborator

Nice! good tip

const mutationId = this.mostRecentMutationId;

try {
const response = await this.mutate();

This comment has been minimized.

@kiurchv

kiurchv Feb 1, 2018

Could we return the result when this.mutate was resolved?


try {
const response = await this.mutate();
this.onCompletedMutation(response, mutationId);

This comment has been minimized.

@leoasis

leoasis Feb 1, 2018

Collaborator

I think we should run this outside of the try/catch. Otherwise, if there's an error inside this method (which is likely since it calls user code), it would trigger the mutation error path, which would be incorrect.

This comment has been minimized.

@excitement-engineer

excitement-engineer Feb 4, 2018

Author Collaborator

Good point. Thanks, updated it.

mutation: DocumentNode;
variables?: TVariables;
optimisticResponse?: Object;
refetchQueries?: string[] | PureQueryOptions[];

This comment has been minimized.

@leoasis

leoasis Feb 2, 2018

Collaborator

@jbaxleyiii if we want to get rid of the query recycler, should we also get rid of the string array in this option? This would look up the currently watched queries by name, which seems to be the same thing that the deprecated updateQueries does.

This comment has been minimized.

@jbaxleyiii

jbaxleyiii Feb 9, 2018

Member

@leoasis yes for sure!

variables?: TVariables;
optimisticResponse?: Object;
refetchQueries?: string[] | PureQueryOptions[];
update?: MutationUpdaterFn;

This comment has been minimized.

@leoasis

leoasis Feb 2, 2018

Collaborator

I'm wondering if we should improve the type of the updater function so that it is parametric on TData, so that the second argument provides the result in the shape you'd expect, instead of an object of any keys of any value.
We could do this here in react apollo, the same way we did it to improve some of the Query types, and then we can send some PRs to correctly fix this in apollo client.

This comment has been minimized.

@excitement-engineer

excitement-engineer Feb 4, 2018

Author Collaborator

I tried to improve on the type and made it parametric on TData, the second argument is now fully typed. The issue with this is that it is not so easy to to fix this issue. I had to override the type definition for ExecutionResult which is contained in graphql-js. So we will have to update the type in that repo to create a long-term sustainable solution I think,

update,
} = this.props;

const response = await this.client.mutate({

This comment has been minimized.

@lifeiscontent

lifeiscontent Feb 3, 2018

why not return the await'd mutate function instead of creating a variable?

This comment has been minimized.

@excitement-engineer

excitement-engineer Feb 14, 2018

Author Collaborator

Good point! I updated the PR:) Thanks

@thomasdashney

This comment has been minimized.

Copy link

thomasdashney commented Feb 7, 2018

I agree with @lifeiscontent - I think it's important for the mutate function to take mutation variables for the following reasons:

  • It's very possible that the variables come from a child component (e.g. using the Formik library):
<Mutation mutation={mutation}>
  {({ mutate }) => (
    <Formik onSubmit={(formValues) => mutate({ variables: formValues })}
      {/* etc */}
    </Formik>
  }}
</Mutation>
  • The existing HOC version's mutate function also takes variables.
@jbaxleyiii

This comment has been minimized.

Copy link
Member

jbaxleyiii commented Feb 9, 2018

@excitement-engineer I think the only question remaining here is the usage of variables. Is it better done as a prop or passed to the function or both (i.e. assign). This is def a case where I think the prop remapping of the HOC gets it right and would be nice to replicate the ideas of that here in some manner?

@JonathanZWhite

This comment has been minimized.

Copy link

JonathanZWhite commented Feb 13, 2018

@excitement-engineer I'm really excited about your work on the Mutation Component! Will this be included in the non-beta 2.1 release? 🎉

@TheGrimSilence

This comment has been minimized.

Copy link

TheGrimSilence commented Feb 14, 2018

@JonathanZWhite I'm hoping

@excitement-engineer

This comment has been minimized.

Copy link
Collaborator Author

excitement-engineer commented Feb 14, 2018

Hey @JonathanZWhite, nice to hear that you like the component! As far as I'm aware this will not be included in the 2.1 release. We will however do our best to get it in your hands as quickly as possible:)

@JonathanZWhite

This comment has been minimized.

Copy link

JonathanZWhite commented Feb 14, 2018

@excitement-engineer Awesome! Thanks for letting me know 🤘

@excitement-engineer

This comment has been minimized.

Copy link
Collaborator Author

excitement-engineer commented Feb 14, 2018

I updated the PR! It is now possible to pass variables as options in the mutation function.

<Mutation mutation={mutation}>
      {(createTodo, result) => {
          setTimeout(() => {
            createTodo({ variables });
          });
        return <div />;
      }}
    </Mutation>

Thanks @thomasdashney for providing a use case with formik where passing variables in the props doesn't work

James Baxley added some commits Feb 16, 2018

James Baxley
James Baxley

@jbaxleyiii jbaxleyiii changed the base branch from master to mutation-component Feb 16, 2018

James Baxley

@jbaxleyiii jbaxleyiii merged commit f433edf into apollographql:mutation-component Feb 16, 2018

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
CLA Author has signed the Meteor CLA.
Details
@jbaxleyiii

This comment has been minimized.

Copy link
Member

jbaxleyiii commented Feb 16, 2018

@excitement-engineer I've merged this into a branch on the upstream so I can finish it out! :yay:

@JonathanZWhite

This comment has been minimized.

Copy link

JonathanZWhite commented Feb 16, 2018

@jbaxleyiii @excitement-engineer Do you see mutation components replacing the HOC mutation pattern completely? Or are there still going to be use-cases for both? In the latter case, what are some good guidelines to figure out when a component should be used vs a HOC?

jbaxleyiii pushed a commit that referenced this pull request Feb 17, 2018

James Baxley
Mutation component (#1683)
* Mutation component (#1520)

* Initial implementation for Mutation component

* Updated the error message

* Improved the component. Added some initial tests

* Added more props to the mutation component.

* Improved typescript typings

* fixed listing issues.

* Added onComplete and onError props

* Implemented logic for handling changes in props.

* Changelog

* prop-types validation

* The render prop only shows the result of the most recent mutation in flight

* Only verify document if the mutation has updated

* Moved the onCompletedMutation outside the try-catch

* Improved the type for the update prop.

* Allow for passing variables as options in the mutation function instead of through the props.

* removed all  types for mutation component

* return result from mutate call

* remove default data type

* bundle size increase for mutation component
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment