API simplifications

[mquandalle]

I find the API to expose Apollo state to UI components too complex, especially considering that this is an operation we’ll have to do for almost every single component. So I’ll start with an example and share some iterative improvement ideas; here it is:

const TopicHeader = connect({
  mapQueriesToProps({ ownProps }) {
    return {
      topic: {
        query: gql`
          query getTopic ($topicId: ID) {
            oneTopic(id: $topicId) {
              id
              category_id
              title
            }
          }
        `,
        variables: {
          topicId: ownProps.topicId,
        }
      }
    }
  },
})(RawTopicHeader);

It’s clear from this example that a lot of code is duplicated, but there is actually a good reason for that which is that some symbols live in the JS world and some in the GraphQL world (eg { query: 'query ...' }). And that’s a hard problem in term of API design. For instance we could try to remove the GraphQL line query getTopic ($topicId: ID) { and have Apollo generate this wrapper automatically with the informations from the JavaScript world, but that’s wouldn’t be suitable because the abstraction doesn’t match in a 1..1 relation (JS doesn’t have types for instance). I just wanted to emphasize this problem because other similar in-scope libraries like OM/next doesn’t encounter it. They use the same language (ClojureScript) for the wrapper file and the query specification, and that allows them to create a perfect API with no duplicate symbols.

So back to my above example, I still think that there is room for improvements, and I’ll try to iterate on it but I may be wrong at some specific step so my goal is more to open a discussion than to provide a concrete proposal.

The first thing I would like to remove is the topic JavaScript variable as I believe we could rely exclusively on the GraphQL names as we do for non-top-levels names like category_id or title. Since in this specific example the JS name (topic) and the GQL name (oneTopic) doesn’t match we need to rename the field in GraphQL:

const TopicHeader = connect({
  mapQueriesToProps({ ownProps }) {
    return {
      query: gql`
        query getTopic ($topicId: ID) {
          topic: oneTopic(id: $topicId) {
            id
            category_id
            title
          }
        }
      `,
      variables: {
        topicId: ownProps.topicId,
      }
    }
  },
})(RawTopicHeader);

The rename really isn’t anything fancy, it’s what we would do for any inner GraphQL field anyway, so it’s more consistent to do it for this top-level name as well. Removing the symbol in the JS space also doesn’t remove anything in term of code editor features because the symbol will be exposed as a component argument as it was prior to this change:

const RawTopicHeader = ({ topic }) => {
  return (<div><h1>{topic.title}</h1></div>)
}

Generally speaking I think that using function arguments as the fence between JS and GQL symbols would be a useful API principle.

The next step might be more controversial but I believe there is some value is switching to a static query instead of dynamically computing it—which should be exclusive to the “variables” part. Static data requirements would be useful to various dev tools and to compose queries without instantiating the related components. So basically we would write something like this:

const TopicHeader = exposeGQL({
  query: gql`
    query getTopic ($topicId: ID) {
      topic: oneTopic(id: $topicId) {
        id
        category_id
        title
      }
    }
  `,
  variables: function(ownProps) {
    return {
      topicId: ownProps.topicId,
    };
  }
})(RawTopicHeader);

Only the variables part is computed, the query is static. I’ll do a little digression to address the (rare?) cases where the data specification depends on some component props, for instance:

const UserName = connect({
  mapQueriesToProps({ ownProps }) {
    if (ownProps.anonymousUser) {
      return {};
    } else {
      return {
        query: gql`
          query getUser ($userId: String!) {
            user(id: $userId) {
              name
            }
          }
        `,
        variables: {
          userId: ownProps.userId,
        }
      }
    }
  },
})(RawUserName);

Here if the user is anonymous we don’t want to send a query to the server asking the user name because we already now that this information doesn’t exist. I can think of two solutions to handle this case:

1. Use the @skip and @include directives, that would be something like

   const Username = exposeGQL({
     query: gql`
       query getUser ($skipFetching: Boolean!, $userId: String!) {
         user(id: $userId) @skip(if: $skipFetching) {
           name
         }
       }
     `,
     variables: function(ownProps) {
       return {
         skipFetching: ownProps.anonymousUser
         userId: ownProps.userId,
       };
     }
   })(RawUsername);

   I’m not super fan of this solution as it sounds a bit like cheating: instead of writing a simple if condition, we have to introduce a weird GraphQL directive to express statically that’s we want to skip a query when executed;

2. Another possibility would be to not specify that we want to skip the query fetching and let the Apollo client figuring that out for us, as follows:

   const Username = exposeGQL({
     query: gql`
       query getUser ($userId: String!) {
         user(id: $userId)
           name
         }
       }
     `,
     variables: function(ownProps) {
       return {
         userId: ownProps.anonymousUser ? null : ownProps.userId,
       };
     }
   })(RawUsername);

   Here we don’t explicitly say that we want to skip the user fetching, but as we don’t pass a valid userId to the GraphQL query (we are passing null whereas a string is expected), there is no way the GraphQL server will return a user from that invalid query and so the Apollo client could avoid the query roundtrip. Consequently the user will be undefined in the UI component, which is what we want in this case.

I don’t want to expend too much on this particular issue of expressing dynamic requirements with static queries (it’s already a big parenthesis), there are probably many other solutions and I believe that a majority (all?) of UI components could express their data requirements in a static way.

Back to the original example, here is the code as we left it before the digression:

const TopicHeader = exposeGQL({
  query: gql`
    query getTopic ($topicId: ID) {
      topic: oneTopic(id: $topicId) {
        id
        category_id
        title
      }
    }
  `,
  variables: function(ownProps) {
    return {
      topicId: ownProps.topicId,
    };
  }
})(RawTopicHeader);

To avoid repeating the world query twice, we could simply switch to ordered function arguments. The first argument is the GraphQL query, the second one is the variables mapping—like this:

const TopicHeader = exposeGQL(gql`
  query getTopic ($topicId: ID) {
    topic: oneTopic(id: $topicId) {
      id
      category_id
      title
    }
  }`,
  function(ownProps) {
    return {
      topicId: ownProps.topicId,
    };
  }
)(RawTopicHeader);

and for stylistic concision only, we would use an ES6 arrow function for the variables mapping:

const TopicHeader = exposeGQL(gql`
  query getTopic ($topicId: ID) {
    topic: oneTopic(id: $topicId) {
      id
      category_id
      title
    }
  }`, 
  (props) => ({
    topicId: props.topicId,
  })
)(RawTopicHeader);

At this point we already gained a lot of concision, one last step (that is maybe too much?) would be to make the second argument (the mapping) optional by providing a default value: the identity function ((props) => props) that would expose the components props to the GraphQL query variables. Thus, the mapping function would be skipped for the most simple components. In our case:

const TopicHeader = exposeGQL(gql`
  query getTopic ($topicId: ID) {
    topic: oneTopic(id: $topicId) {
      id
      category_id
      title
    }
  }
`)(RawTopicHeader);

And that’s it, I’m pretty happy with this last snippet :-)

I’m sorry for the very long text, I thought it was useful to share my thought process to facilitate the discussion about potential API simplifications.

[jbaxleyiii]

@mquandalle thank you so much for such a detailed outline of improvements!

Overall I'm a really big fan of these improvements! I do think that statically setting the query could be really great and could even make it easier for a pre-processing tool like webpack to use .gql files to build the query. I also don't think that manipulating the GraphQL string is a good thing to do, strings don't make good ORMs haha.

I also like the argument restructure making the first argument the string and the second the function returning the dynamic data. This is more akin to how relay works but we wouldn't require preprocessing. I think both of the above things work well with mutations as well.

The problem I see is in wanting to make multiple queries for a single component. Although GraphQL allows for combining multiple queries, there are cases when you want to have different individual queries that this would remove support for. For instance, you may want to refetch on parts of the containers data. If this is in a separate query, it would be easy to do. In the API above, you can only refetch everything or write a new query that is a one off query.

The other downside is the removal of the connect wrapping of react-redux. If the solution is better than I'm open to that (not all apollo react apps use redux) but I do really like that aspect of this client

@stubailo what are your thoughts on this?

@mquandalle I'll think through a couple possible solutions to above to see if we can make this better overall

[jbaxleyiii]

also:

At this point we already gained a lot of concision, one last step (that is maybe too much?) would be to make the second argument (the mapping) optional by providing a default value: the identity function ((props) => props) that would expose the components props to the GraphQL query variables. Thus, the mapping function would be skipped for the most simple components.

🔥 🔥 🔥

[mquandalle]

@jbaxleyiii One possible solution for multiple queries would be function composition:

const connectTopic = exposeQGL(gql`
  query getTopic ($topicId: String!) {
    topic: oneTopic(id: $topicId) {
      id
      category_id
      title
    }
  }
`);

const connectUser = exposeQGL(gql`
  query getUser ($userId: String!) {
     ...
  }
`);

export const Component = connectTopic(connectUser(RawComponent));

[mquandalle]

I’m not sure about the advantages of re-introducing an explicit call to Redux’s connect, but that could certainly be done. Instead of having a exposeQGL function calling connect under the hood, we would export a apolloConnector function and let the user givie its result as an input to connect:

import { apolloConnector } from 'react-apollo';
import { connect } from 'react-redux';

const topicConnector = apolloConnector(gql`
  query getTopic ($topicId: ID) {
       ...
    }
  }`, 
  (props) => ({
    topicId: props.topic._id.toUpperCase(),
  })
);

const TopicHeader = connect(topicConnector)(RawTopicHeader);

But I don’t understand what we gain.

[jbaxleyiii]

@mquandalle the benefit of combining redux is reducing the number of compositions and making it easier to adapt for redux apps while still providing a good api for non redux apps.

One possible solution for multiple queries would be function composition

This would work but I think it adds back in the boilerplate we were seeking to reduce? However it could allow you to chain queries so you can pass the state / result from one query to another.

[jbaxleyiii]

We could introduce something akin to this if you are doing multiple queries:

import { apolloConnector,  graphql, combineRequests } from 'react-apollo';

const connectTopic = graphql(`
  query getTopic ($topicId: String!) {
    topic: oneTopic(id: $topicId) {
      id
      category_id
      title
    }
  }
`);

const connectUser = graphql(`
  query getUser ($userId: String!) {
     ...
  }
`);

export const Component = combineRequests([connectTopic, connectUser])(RawComponent);

This would pass the query keys as props to the RawComponent. We would need to throw or at least warn on like keys but that seems easy enough.

import { graphql } from 'react-apollo';
import { connect } from 'react-redux';

const topicConnector = graphql(`
  query getTopic ($topicId: ID) {
       ...
    }
  }`, 
  (props) => ({
    topicId: props.topic._id.toUpperCase(),
  })
);

const mapStateToProps = (state) => {
  foo: state.foo
}

const TopicHeader = connect(mapStateToProps)(topicConnector)(RawTopicHeader);

This is a pretty significant api change. We certainly aren't very far into this so I'm open to the change for sure! I'd love to get some more thoughts on this from people too.

@johnthepink @stubailo any thoughts?

[jbaxleyiii]

After reviewing the current API, I'm not sure I think it's too much boilerplate. Removing the key definition for what is in the query means we have to parse each query from react-apollo instead of letting the client do all of that internally. The argument as a single object makes itterative API additions much easier and safer, and dynamically changing queries seems like more common use case than I originally thought.

[johnthepink]

Personally I'm good with the current API. 👍

[stubailo]

I think these are pretty similar conversations, right? #30

So we did end up with a few simplifications.

[mquandalle]

The other downside is the removal of the connect wrapping of react-redux.

This is actually something I find really weird. React-Apollo imitates Redux’s connect by exporting a function with an identical name but with a different signature:

React-Redux: connect([mapStateToProps], [mapDispatchToProps], [mergeProps], [options])
React-Apollo: connect(options)

With that, how do I connect my client Redux store and Apollo data in the same component? Do I have to import the two connects functions and rename one of them,

import { connect as connectRedux } from 'react-redux';
import { connect } from 'react-apollo';

or do I have to use only the Apollo connect but change all my Redux’s connect calls to replace positioned arguments by named arguments?

If one of the goal of this package is to integrate with a “end-developer” Redux store, then it should use Redux connect and not a “wrapper”.

[mquandalle]

The argument as a single object makes itterative API additions much easier and safer

Good point. In this case assuming that we use a custom connector function like the exposeQGL one I used in the original post, we could change it’s signature from:

exposeGQL(query, mapping);

to

exposeGQL(query, options);

where options would basically contains all the current options like forceFetch or returnPartialData and the mappings from props to GraphQL variables as well. I also don’t really like the function name exposeGQL but I still think it could reduce the complexity involved in Apollo and React connections.

[tlvenn]

Any follow up on this topic ? I do share @mquandalle concern over the usage of connect especially when not using Redux with React. It's maybe nice to have a 2 in 1 connect in a Redux app but I feel it should be optional and the end user shouldn't really have Redux bleeding in his app if he is not using it at all.

[stubailo]

I don't see why using a function called connect would be a problem if you're not using Redux. Just ignore the Redux part and you're good to go!

[tlvenn]

Hi @stubailo,

It's not really problem, we are just mostly debating over names here..

connect is a strong reminiscence of Redux and it actually does provide Redux. When I see connect, I see Redux, not React for Apollo. Imho, what you use underneath to power Apollo should ideally not surface so much to the end user unless it makes sense and it does only when the end user is also using Redux.

In a non Redux app, I would prefer to have another function to bind Apollo client to my React components which does not expose Redux at all.

[stubailo]

I would prefer to have another function to bind Apollo client to my React components which does not expose Redux at all.

How would this be different from just not using the parts of connect you don't want?

[tlvenn]

It would sandbox away Redux so that no one suddenly decide it would be a good idea to leverage redux in a given component just because they can... The name of that function would also hopefully convey better that we are dealing with Apollo / GraphQL, something that exposeGQL is doing for me.

I have nothing against Redux, it's a great lib but imho it should not be in my face so to speak when I don't use it to begin with and only want to integrate Apollo Client, a GraphQL client with React.

[tlvenn]

The way it is now give me the impression that I am using Redux with a Apollo Client middleware while it should be the other way around.

[stubailo]

I think it would be pretty simple to make a wrapper for react-apollo that makes a simpler connect API that doesn't have the redux stuff. Would you be interested in working on that?

[tlvenn]

I can definitely look into it but is there any chance we can not name it connect ?

[stubailo]

Yeah I mean if you make a package for it you can call it whatever you want. There's no reason there couldn't be two equally supported React integrations for Apollo, with different APIs and whatnot.

[tlvenn]

For my own sake for sure but I was kinda trying to convince you that it was not necessarily a good idea to expose Redux upfront by default the way the Apollo Client for React lib is currently doing.

[jbaxleyiii]

After some further time to let this sit, here is what I'm thinking (that I hope gives everyone a win 👍)

import { connect } from "react-apollo" stays the default export / connect method. There is a large amount of community projects that use react + redux and I think keeping a single method of connect is still really helpful. I have heard from quite a few developers that the sameish signature has made migrating an app much easier for them.

import { graphql } from "react-apollo/container" // also exports combineRequests and Provider

react-apollo/container

The /container is a version of react-apollo that does not rely on react-redux at all. It favors functional composition and exports a combineRequests method to help make that easier.

import { graphql, combineRequests } from 'react-apollo';
import RawComponent from './RawComponent';

const connectTopic = graphql(gql`
  query getTopic ($topicId: String!) {
    topic: oneTopic(id: $topicId) {
      id
      category_id
      title
    }
  }
`, 
  (state, props) => ({
    variables:  {
      topicId: topicId: props.topic._id.toUpperCase(),
    },
    forceFetch: true,
    pollInterval: 60000,
  })
);

const connectUser = graphql(`
  mutation createUser ($userId: String!) {
     ...
  }
`, (state, props, ...args) => ({ userId: state.user + args[0] })
);

export const Component = combineRequests([connectTopic, connectUser])(RawComponent);

The signature of graphql is graphql(Document, () => QueryHandleOpts || MutataionHandleOpts). Internally we determine if the action is a query or is a mutation (only one allowed per graphql) and only pass the correct data (variables vs variables && forceFetch && pollInterval && returnPartialData).

const TopicHeader = graphql(gql`
  query getTopic ($topicId: ID) {
    topic: oneTopic(id: $topicId) {
      id
    }
  }
`)(RawTopicHeader);

// would result in the following props inside RawTopicHeader
this.props.topic // storage location

// initial state
{
  loading: true,
  errors: false,
  refetch: Function
}

// with data
{
  loading: false,
  errors: false,
  refetch: Function,
  topic: {
    id: XXX
  }
}

// mutations
const TopicHeader = graphql(gql`
  mutation createTopic ($topicId: ID) {
    createTopic(topicId: $topicId) {
      id
    }
  }
`, (state, props, ...args) => ({ userId: state.user + args[0] })
)(RawTopicHeader);

// would result in the following props inside RawTopicHeader
this.props.createTopic // storage location

// initial state
{
  loading: false,
  errors: false,
  call: Function(args)
}

// with data
{
  loading: false,
  errors: false,
  call: Function(args),
  createTopic: {
    id: XXX
  }
}

// to call
this.props.createTopic.call(/* args */)

Ramifications

Sometimes this feels like two libraries in one. I am totally good to maintain both APIs (I like this one a lot for non redux projects). Personally, I'd like one react-apollo library instead of a number of different versions. I think it breeds more confusion for beginners and increases surface area for improvements. I think we will need clarity on when to use either version or in the very least, clear documentation for both.

Fwiw, I think this API is more advanced (removing some clarity for composability and succinctness) which isn't a bad thing, but shouldn't be the default of this library.

All in all I'm game for this. Thoughts @stubailo @johnthepink @mquandalle @tlvenn?

[stubailo]

Personally I'm not yet convinced that having this different API will make or break Apollo client usage among react applications. Do you think one API could be implemented in terms of the other? It doesn't seem worth it to me for us to maintain two different ones, the better alternative I think would be to give people the tools to make whatever API they want, perhaps by moving some of the prop diffing and caching stuff into Apollo client core.

[tlvenn]

Thanks a lot for this proposal @jbaxleyiii ! For me, this is pretty much what the Apollo API for React should look like. I believe @mquandalle might be able to give you better feedback on the details of this proposal but it seems to capture most of his ideas.

@stubailo another thing that is important to consider is that by shielding the details of the implementation (Having a public API that is not coupled with Redux), should you need to get rid of Redux at some point, it will not affect users at all.

[jbaxleyiii]

@stubailo as we work to make apollo-client easier to use with native redux implementations, I could see the current implementation becoming redundant. Whereas, this API strips the redux stuff out totally but could be composed with redux quite well. For instance:

#56 (comment) and #62 and even apollographql/apollo-client#244 (granted this is just a read method, not any actual bindings to setup things)

I like both so either / both is a win for me 😄

[tlvenn]

Is there any consensus reached here ? For non redux app, I definitely think that @jbaxleyiii API proposal is best and I hope given the momentum mobx is currently having, you will see even more value in such API. @stubailo any more though on this topic ?

[stubailo]

I quite like @jbaxleyiii's idea. Perhaps it is good to have single-purpose containers rather than trying to have a single mega-container that handles everything. Does the current container approach have any advantages over this new syntax? Perhaps we should switch over completely.

I also really like the idea of using the root fields on the query as the prop names, since you can use aliases to change them quite easily.

[jbaxleyiii]

@mquandalle @tlvenn @stubailo @johnthepink I'm going to be making these changes starting in two weeks. I'm going to ship SSR, then ship a feature for NewSpring (my job haha), then ship the new api

Thank you all for the great input!

[jbaxleyiii]

I'm planning on starting this build on friday for all of those interested. Any volunteers to test release candidates?

[tlvenn]

Sure thing, count me in

[tmeasday]

I like the direction of this thread although I have a couple of things to add.

Querying

I was always confused by the name mapQueriesToProps -- and thinking about it again, I think I understand why. In vanilla redux, state comes into the store somehow (not relevant to the HO connecting component), and then the component takes the store's state and turns it into props for its wrapped child (the "inner props"). Thus the name mapStateToProps.

Here, we are doing a two things:
a. Taking our outer props, and turning them into queries + variables + options.
b. Taking the output of those queries, and turning them into inner props.

So really I think the HOC should take two distinct functions:
a. query or just options (with a static query): takes out props/state and turns them into queries + opts + variables.
b. mapResultToProps -- this takes the result of the query, and turns it into props for the inner component.

I don't think the inner component should need to understand the queries or anything about react-apollo, so I don't think it should be accessing the topic at this.props.topic.topic et al.

I think instead the API should be like:

const TopicHeader = connectQuery({
  // we could make this an unnamed first argument too I guess?
  query: gql`
    query getTopic ($topicId: ID) {
      oneTopic(id: $topicId) {
        id
      }
    }
  `,
  // no reason not to just use `this` here is there? 
  options() {
    return { topicId: this.props.topicId };
  },
  mapResultToProps(result) {
    if (result.loading) {
      return { topicLoading: true };
    } else {
       return { topic: result.oneTopic, refetchTopic: result.refetch };
    }
  }
})(RawTopicHeader);

Perhaps it seems trite, but I think the API of the wrapped component becomes much more natural. Just like you don't pass Redux's state into a wrapped component and have it concern itself with where its data lives, I don't think we should pass in the query's state directly.

We could probably come up with good default implementations of options (take any variables from the query and pull them off this.props), and mapResultsToProps (similar to mine above maybe?) too, if we think that's not going to obscure what's going on.

Mutations

For mutations, I don't think we should be passing the result of the mutation into the component as a prop. It just doesn't make sense for me for something ephemeral like the result of a mutation to go there--as an example, Redux doesn't let you pass the result of a dispatch into a prop either.

Instead if users need access to that data and the mutation can't just mutate Apollo's store directly, I think it needs to be passed into a callback and then the callback can do whatever the user ordinarily does with such ephemeral state: call this.setState, dispatch it to redux, etc.

So I think the API for mutations should be more like:

const TopicHeader = connectMutations(mutation => {
  createTopic: mutation({
    query: gql`
      mutation createTopic ($topicId: ID) {
        createTopic(topicId: $topicId) {
          id
        }
      }
    `,
     options: (...args) =>  {
        // this here should still be the HOC
        return { userId: this.state.user + args[0] };
     }
   })(RawTopicHeader);

And if we wanted to handle via Redux:

class RawTopicHeader extends Component {
   onButtonPush() {
      this.props.createTopic(this.state.topicTitle)
        .then(error, topic => {
           if (error) {
             // in this case these props would get passed in by a Redux connect `mapDispatchToProps`
             this.props.setCreateTopicError(error);
           } else {
             this.props.addCreatedTopic(topic);
           }
        });
   }
}

(I realise this is almost identical to the current mutation API btw. I'm inclined to think it's clearer to have a function mutation that produces the prop that the inner component calls feels more natural than passing an object to mapMutationsToProps and have it happen for me)

[jbaxleyiii]

PR for new API #120.

The changes are all documented on the PR

[stubailo]

Hooray!