klis87 / redux-requests

Declarative AJAX requests and automatic network state management for single-page applications
https://redux-requests.klisiczynski.com/
MIT License
367 stars 51 forks source link

Add mutation data to requests reducer #521

Open Nerlin opened 2 years ago

Nerlin commented 2 years ago

About

This pull request exposes mutation data in the requests reducer making getMutation and getMutationSelector functions more useful for side-effects like redirects:

For example:

import { connect } from "react-redux";
import { getMutation } from "redux-requests/core";
import { Navigate } from "react-router-dom";

export function NewProductPage({ data, loading, error, onSubmit }) {
  if (data) {
    return <Navigate to={`/products/${data.id}`} />
  }
  return <ProductForm loading={loading} error={error} onSubmit={onSubmit} />
}

// action creator
const createProduct = ({ data }) => ({
  type: "CREATE_PRODUCT",
  request: {
    url: "/api/products/",
    method: "POST",
    data,
  }
});

const mapStateToProps = (state) => {
  const { loading, error, data } = getMutation(state, { type: "CREATE_PRODUCT" });
  return { loading, error, data };
}

const mapDispatchToProps = {
  onSubmit: createProduct
};

export default connect(mapStateToProps, mapDispatchToProps)(NewProductPage);

To do a redirect in NewProductPage component, the code must know the product id returned by the server. The response data is saved in the state for queries only making it hard to implement this behavior without hooks.

Analogies

Many other popular libraries provide this functionality:

Pitfalls

The PR affects TypeScript definitions in @redux-requests/react library changing generic types used for MutationProps, MutationCustomComponentProps and Mutation types. This may be considered as a breaking change since the first generic type represents MutationState data now similar to QueryState.

klis87 commented 2 years ago

Thank you for this PR. I have a question though, if we keep data of a mutation in the redux store, what is the difference between query and mutation then? Just a though, could we instead use asMutation: false? Please see https://redux-requests.klisiczynski.com/docs/api-reference/request-action#asmutation-boolean

One of the mentioned use cases is actually yours:

Also could be handy if your want to treat a mutation (for example POST request) as a query for instance to store its data.

Nerlin commented 2 years ago

Thank you for your review. In terms of command-query separation principles, commands should not return a value indeed. But when one mentions queries and mutations in the modern frontend world, most people probably think about GraphQL which made these terms popular. In GraphQL, mutations are requests that causing some side-effects on the server, meaning that any operation that causes writes should be named a mutation: https://graphql.org/learn/queries/#mutations

It doesn't mean that mutations cannot query data though. Following CQS-principles for things like client-service communications might be not useful in practice where in most cases we want to get results of atomic transactions made by the client.

As for the asMutation flag, thanks for this reference. This should probably work in my case but maybe the approach that I'm trying to explain here would be more obvious for other developers. I couldn't find an example how to do that in the docs, nor an API reference in the mutation docs, but the task of getting mutation result looks quite typical.

klis87 commented 2 years ago

Thank for you answer. Regarding mutation, you still can get its result by awaiting request dispatch (https://redux-requests.klisiczynski.com/docs/guides/actions#promisified-dispatches). It is not saved anywhere though, so useMutation hook cannot return it at the moment. So, if asMutation: false works in your case, I would really prefer to be practical and, at least in the current major version, to leave it as it is. We could document it better though so other people could find it easier, do you think some ideas what could be a good place?

I hope that you did not spend too much time on this PR, I really appreciate it, the reason I want to keep current version as it is right now is that I am working on the version 2, which will have very big internal improvements (with more or less the same API though). But the refactoring is huge, especially about normalization logic.

Btw, it is also totally cool to raise an issue first, before doing a PR, so then we could decide if a use case is solvable by the current version already. Thanks again!

Nerlin commented 2 years ago

Awaiting the dispatch might be sufficient for this, thanks. I understand that this PR looks quite big and may affect your future plans. I didn't spend too much time on it so I don't mind if we simply update the docs. The PR can be kept somewhere in an archive as an idea, if later you'll find that it would be convenient to save mutation results.

Regarding the docs, I believe it would be great if the docs would provide an example how things like getting an id of a newly created product can be achieved with the current API. It might be not so simple to make a convenient example without mentioning React though, but might be something like this:

async function createProduct(data) {
  const product = await dispatch({
    type: "CREATE_PRODUCT",
    request: {
      url: "/api/products/",
      method: "POST",
      data,
    }
  });
  return product.id;
}

Also, it might be worth mentioning in the API reference for getMutation that mutation data is not saved but actually available by using promisified dispatches. Finally, the mutations example here can be updated with some example as well. Or, as an alternative, another example can be added here: https://redux-requests.klisiczynski.com/docs/introduction/examples, it just needs a good name ("promisified dispatches" could be confusing for some people).

Thanks again for reviewing this. Feel free to close the PR.