Skip to content
This repository has been archived by the owner on Apr 13, 2023. It is now read-only.

Intial build #1

Merged
merged 17 commits into from
Apr 16, 2016
Merged

Intial build #1

merged 17 commits into from
Apr 16, 2016

Conversation

jbaxleyiii
Copy link
Contributor

@jbaxleyiii jbaxleyiii commented Apr 15, 2016

Design implementation is in the README. 🎉

We need to go ahead and setup CI / Coverage and the deps on apollo-client will need to be updated after the next publish.

This needs more tests too, but all functionality should be there so we can start using it while I back test all of it

cc @johnthepink @stubailo

@@ -0,0 +1,57 @@
# Design principles of the Apollo Client
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extra file!


const mapStateToProps = ({ foo, baz }) => ({ foo, baz });

@ReactReduxConnect(mapStateToProps)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoa, what is this decorator magic?? I had no idea you could use react-redux in this way!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah!! Its how we do everything! It's sooooo clean

@stubailo
Copy link
Contributor

I went ahead and renamed the repo to react-apollo, I think that's the right convention based on react-redux.

@jbaxleyiii
Copy link
Contributor Author

@stubailo:

Here are some thoughts on a slight tweak to the query API and the mutation API.

Queries

Currently the API has a watchQuery method that is passed to the user to use in order to create queries. The build a dictionary of [key: string]: WatchQueryHandles that gets mapped to the props passed to the wrapped component. The adjustments below have the user creating a dictionary of [key: string]: WatchQueryOptions. The reason for doing this is two fold. 1st the connect component decides how to get data from the store. Behind the scenes it will be calling watchQuery but in the future if we wanted to add / make changes behind the scenes we could call different methods for different data (not sure if useful). The real benefit in my option is the WatchQueryOptions. Since the watchQuery handle isn't called when getting the dictionary anymore, we can get the dictionary in componentWillMount, use the WatchQueryOptions to see if we already have all of the data using getFromStore methods and if we do, the intial state passed to the wrapped component would be the full data + no errors + not loading. Then we could bind the watchQuery on componentDidMount for reactivity purposes. If we don't have the data, we can easily pass default data { loading: false, error: null, result: null } on componentWillMount and create the query on componentDidMount. Doing this also allows us to extend and when rendering on the server, we can call client.query(WatchQueryOptions) and use the resulting promise to delay SSR until the data has returned and prefill the store.

Mutations

Similar to the changes in query, mutations return a dictionary of [key: string]: MutationOptions which is used to bind the resulting props. The prop of each key represents a custom function (with as many args as wanted) which calls client.mutate(options) behind the scenes. The key on the props also represents the state of props in the same shape as the query props ({ loading: false, error: null, result: null }). We could also add a hasBeenCalled: boolean or some other key if wanted but I think between loading, error, and result, a dev should be able to reason that if needed. This gets us the same benefits on the SSR side so something like an analytics mutation component could report SSR page loads or other data if desired. I think the mixed method + object is a little odd but it seems to be the cleanest solution overall

Added props

Simillar to redux's @connect, I think that our's should pass dispatch, query, and mutate as props to the wrapped component. That way if custom client actions (one off mutations or queries) are wanted, they are possible. It also means @connect() is a non reactive way to get access to the applications client API.

import { connect } from 'apollo-react';

function mapQueriesToProps({ ownProps, state }) {
  return {
    category: {
      query: `
        query getCategory($categoryId: Int!) {
          category(id: $categoryId) {
            name
            color
          }
        }
      `,
      variables: {
        categoryId: 5,
      },
      forceFetch: false,
      returnPartialData: true,
    }
  }
}

function mapMutationsToProps({ ownProps, state }) {
  return {
    addCategory(/* args */) {
      return {
        mutation: `
          mutation postReply(
            $topic_id: ID!
            $category_id: ID!
            $raw: String!
          ) {
            createPost(
              topic_id: $topic_id
              category: $category_id
              raw: $raw
            ) {
              id
              cooked
            }
          }
        `,
        variables: {
          // Use the container component's props
          topic_id: ownProps.topic_id,

          // Use the redux state
          category_id: state.selectedCategory,

          // Use an argument passed from the callback
          /* args */,
        }
      };
    },
    otherAction(controlArg1, controlArg2) {
      let mutation = `
          mutation postReply(
            $topic_id: ID!
            $category_id: ID!
            $raw: String!
          ) {
            createPost(
              topic_id: $topic_id
              category: $category_id
              raw: $raw
            ) {
              id
              cooked
            }
          }
        `
      if (controlArg1) {
        mutation = `
          different mutation
        `
      }

      return {
        mutation,
        variables: {
          // Use the container component's props
          topic_id: ownProps.topic_id,

          // Use the redux state
          category_id: state.selectedCategory,

          // Use an argument passed from the callback
          /* args */,
        }
      };
    },
  }
}

@connect({ mapQueriesToProps, mapMutationsToProps })
class AddCategory extends Component {

  onAddCategoryClick(e) {
    const { target } = e;
    const { addCategory } = this.props;

    this.props.addCategory(target.id, target.value);
  }

  onOtherAction(e) {
    const { target } = e;
    const { otherAction } = this.props;

    this.props.otherAction(target.id, target.value);
  }

  onCustomComponentMutation(mutation, variables) {

    this.props.mutate({ mutation, variables })
      .then((graphQLResult) => {
        const { errors, data } = graphQLResult;

        if (data) {
          console.log('got data', data);
        }

        if (errors) {
          console.log('got some GraphQL execution errors', errors);
        }
      }).catch((error) => {
        console.log('there was an error sending the query', error);
      });

  }

  onCustomQuery(query, variables) {
    this.props.query({ query, variables })
      .then((graphQLResult) => {
        const { errors, data } = graphQLResult;

        if (data) {
          console.log('got data', data);
        }

        if (errors) {
          console.log('got some GraphQL execution errors', errors);
        }
      }).catch((error) => {
        console.log('there was an error sending the query', error);
      });
  }

  render() {

    /*

      this.props.addCategory is a function that calls a mutation and has the added data of:

      {
        loading: boolean,
        error: Error,
        result: GraphQLResult,
      }

      this.props.categories is a just an object representing the query:

      {
        loading: boolean,
        error: Error,
        result: GraphQLResult,
      }

    */

  }

}

Thoughts?

cc @johnthepink who built our current react + graphql mixin

@jbaxleyiii
Copy link
Contributor Author

jbaxleyiii commented Apr 16, 2016

I am interested to see how props that are both a method and data trigger updates on children components. Will this.props.foo === nextProps.foo since the method is the same? I'm afraid it will which means the shared method + data won't really work.

If it doesn't I would probably propose this.props.mutations[key]: call mutation and leave this.props[key] to be the data just like it is with query.

That would bring us up to 4 passed props (dispatch, query, mutate, mutations) if mapMutationsToProps is used

@stubailo
Copy link
Contributor

@jbaxleyiii:

The query stuff looks great. I've been thinking about building in synchronous ways to get initial data into Apollo Client directly, but implementing it in the React container first would be great, and then we can move to the actual feature when it exists.

Re: mutations

I think the mixed method + object is a little odd but it seems to be the cleanest solution overall

I think you're right that putting the props on a function is a bit odd. Perhaps we have an extra option in MutationOptions called statusProp which tells the container that the status of the mutation should be put in a specific prop?

So like:

function mapMutationsToProps({ ownProps, state }) {
  return {
    addCategory(/* args */) {
      return {
        mutation: `...`,
        variables: {...},
        statusProp: 'addCategoryStatus',
      };
    },
  }
}

// In the component

this.props.addCategoryStatus.loading

Although, I'm also OK with your proposed design! Up to you.

Re: added props

I think this is a great idea!

@jbaxleyiii
Copy link
Contributor Author

@stubailo thanks for the feedback! I should have this done tomorrow! I've also got to make the component not require a redux store. I'll open a PR to the docs repo once this is merged.

@jbaxleyiii jbaxleyiii changed the title [WIP] Intial build Intial build Apr 16, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants