Design for easy and consistent view integration

[tmeasday]

This is a proposal to centralize the "state" management part of Apollo Client integrations inside the JavaScript client.

State of the art

I'll speak chiefly about the react-apollo integration, although it appears the same rationale would apply to the Angular integration also.

The current API for react-apollo adds a layer between the user and apollo-client, in both directions:

• When querying/mutating you call a react-apollo API
• The results are returned to you via a react-apollo API.

  Rationale for change

It seems like there are three main reasons why doing it this way isn't ideal:

1. It's confusing for people to look at the Apollo Client API docs for details of the options etc, and not be looking at the API that they actually call.
2. Client integrations must be continually updated to provide the same shape of API as Apollo Client develops.
3. There's a significant amount of shared logic (for instance, around not re-querying when query arguments don't change) that could be shared between client implementations.

   Basic Design

The basic commonality of any view layer is the concept of a visual component that has changing arguments and state, but is not rendered from scratch each time, but instead tries to reflect the new desired state with minimal work.

So new concept proposed in Apollo Client is the "context". The idea is that each data-aware component in the view layer would have a corresponding Apollo Client context. The context would offer a context.query() API with the following behaviour:

1. The first call to context.query() would always hit the server, even if the results are known.
2. Further calls to context.query() would have no effect if the arguments or variables have not changed (This avoids the integration needing to understand the arguments to query and diff-ing them)
3. There should be a context.teardown() method which would clean up all resources associated with the context (so the integration does not have to understand the resources associated with a component).

   What does a specific integration actually need to do?

4. Set up a context for each relevant component.
5. Handle the component's lifecycle, e.g. calling teardown at the appropriate point.
6. Deal with the observable/promise impedance mismatch between Apollo Client's API and the API of a render function OR
7. Listen to changes in query results and re-render.

   Tom's Proposed react-apollo API

Again, I think that a lot of this would apply to the Angular integration too.

Suppose that context.query() returned the results as we currently understand them (rather than a promise or an observable), and there's (something like) an additional context.on('change') API that the integration can monitor for point 4 above. This is a tracker-ish API I guess.

Then the API for a container could be something like

// takes one argument, conceptually "mapContextToProps"
@graphql((context, ownProps) => {
  const { id, handleError, ...other } = ownProps;
  const results = context.query(MY_QUERY, {
    variables: { id }
  });

  if (results.loading) {
    return { loading: true, ...other };
  } else {
    return {
      feed: results.feed,
      addItem(itemText) {
        return context.mutate(ADD_ITEM_MUTATION, {
          variables: { id, itemText }
        }).then((error, result) => {
          if (error) {
            handleError(error);
          }
        });
      },
      ...other,
    };
  }
});
class Feed extends Component {
  ...
}

Apart from the rationale above, the advantages I see in this API over the current 0.4.0 API are:

1. Straightforward mapping of results to "inner" props. Rather than an awkward third argument to the container component, it's clear how the query results become props. If you want you can just return context.query() yourself as props, which means your @graphql function can be super simple.
2. Easy ability to work mutate into an event handler callback at any point you like in any form you like.

[stubailo]

I know I used the word "context" when we talked about this before, but perhaps we can just call it a "component"? Or is that confusing? Just worried about using a relatively generic word like "context" which could mean almost anything.

Some specific questions:

1. What does context.query return? Is it like watchQuery and returns an observable?
2. I'm not sure what (3) or (4) mean in your list - do you mean that the react integration perhaps wouldn't use the observable return value at all? (OK just saw in your code sample, it kind of makes sense)
3. Does the function that takes context and ownProps re-run whenever either of those change? Could that be too costly?
4. Do we expect people to be able to operate on the query results in an arbitrary way before they are passed down? This seems intuitive, but, again we need to make sure it's not expensive to run this function tons of times.

I like this idea because it seems to match up with the way Meteor subscriptions work, which people really like as far as I know.

You know what's kinda funny - this is somewhat similar to: https://github.com/stubailo/meteor-reactive-method which is also a package which fakes a declarative API over an imperative action.

[stubailo]

Oh, one final question - if the goal here is to unify React/Angular integrations, what does the "Core API" look like? Is it:

const context = client.context();

context.watch(() => {
  const result = context.query(...);
  displayData(result); // whatever custom function you have
});

Almost seems like the context should return an observable here:

observable = context.observe(() => {
  const result = context.query(...);
  return {
    key: result,
  }
});

Then teardown can just be unsubscribe.

[tmeasday]

I agree context is not a good name, I just didn't want to debate that yet. Maybe component query?

1. The problem with it not returning a object, ala Tracker is that then the user needs to manipulate the result in a callback, which makes things more complex and confusing. That's why my feeling is an object is best.
2. Yeah if it returns an object the react wrapper doesn't need to deal with the result.
3. (And 4.) this is the downside of the tracker style approach. If it's a callback it can just run when its inputs change so it's definitely a trade off. However, I think it's ok as long as people separate concerns (would you commonly perform complex manipulations of query results?)

Your API could work and is probably minimally weird.

[abhiaiyer91]

Why diverge from syntax that people are already familiar with via @connect? Both ngRedux and react-redux utilize the connect function to provide a layer of indirection so that your code is less bound to a store instance.

In regards to

1. Core implementation vs View Layer integration is really that confusing? The react integration seems less confusing and maybe it's the way the docs are presented today that causes this confusion.
2. I agree with 2.
3. every view layer has its own quirks. Which makes the integrations powerful, this can allow the users of those framework do things in a "natural" style that they're used to.

If the focus is to unify view layer integration how does this look in Angular2? Interesting to first change React Apollo without considerations to other view layers concurrently?

[abhiaiyer91]

If there are already best practices being built in each respective community, why diverge from that for the sake of creating a new standard ? Shouldn't the goal be to embrace each community, have contributors that are thinking about these problems with the framework in mind, and executing changes when they occur? Or is there not enough volume of those people?

[stubailo]

I guess it really comes down to - do we want to maintain completely separate docs for Angular and React, and make constant changes to those APIs for every new feature in core?

It could be fine, we've been doing that with react-apollo for a while.

If the focus is to unify view layer integration how does this look in Angular2?

One of the goals of this post is to get feedback from Angular devs - if we don't write up a design we can't get the feedback!

But I see what you're saying - if we want to feel like a really natural thing for different view layers, those are going to have different conventions in their community. It probably isn't worth giving up that natural feel to unify the API if that makes it worse for both communities.

BTW - the API we are proposing here is actually more similar to Redux connect, since it basically comes down to using selectors that re-run on store change. The previous API (mapQueriesToProps) was decidedly not like connect, since it didn't let you write your own selectors and transformations. But I think it needs some massaging to get there.

[tmeasday]

@abhiaiyer91

Both ngRedux and react-redux utilize the connect function to provide a layer of indirection so that your code is less bound to a store instance.

I'm interested as to why you mention this--a big part of my thinking behind the "proposed" part of this design is exactly this philosophy. For instance, my major gripe with the current 0.4.0 react-apollo API is that the default props for the wrapped component looks like:

X.props = {
  someQueryName: {
    loading: true,
    field1: data,
    field2: data,
    refetch: function() ..
  }
}

Which means that X at some level needs to understand that its input (field1 in this case) comes from a query called someQueryName and there are all these extra props (like refetch) in there that are likely not relevant to the presentational component.

(It should be noted that @jbaxleyiii added a mapResultsToProps to solve this exact problem, and it's great, but as mentioned above, it'd be nicer not to have multiple callbacks).

I agree with 2.

Which 2 was that? ;)

Interesting to first change React Apollo without considerations to other view layers concurrently?

Actually I did look at the angular integration and it looks conceptually very similar to the React one right now. I'm just not confident enough about Angular terminology to write it up my proposed design in Angular-language yet. I have asked @Urigo to take a close look at this issue though.

If there are already best practices being built in each respective community, why diverge from that for the sake of creating a new standard?

I definitely don't want to do this!

I'm not completely certain what the current best practice in the React community is for data sources with the following properties:

1. Asynchronous (i.e. returns a promise or observable).
2. "Rich" in that what's returned is more than just data, it's also other metdata (like refetch, etc), and the user needs to select what they need.

I'm modelling the above very much on the spirit of redux (which is the direction @jbaxleyiii is going in too, I think). But maybe there's a data access library we should be looking at?

[abhiaiyer91]

Thanks for the clarification @tmeasday

I'm interested as to why you mention this--a big part of my thinking behind the "proposed" part of this design is exactly this philosophy.

Ah because their interaction with connect functions are just different syntaxes. Im just on the side of letting the Angular nerds and React nerds have the same exact api they are already using but enhanced for our purposes.

Which 2 was that? ;)

This number 2 mate!

Client integrations must be continually updated to provide the same shape of API as Apollo Client develops.

Ah I get it now @tmeasday

It just seems this

@graphql((context, ownProps)

Should read

@graphql(mapStateToProps, mapDispatchToProps, mapQueriesToProps, mapMutationsToProps)

and both mapStateToProps and mapResultsToProps take ownProps as their second argument.

Maybe i was lost in translation a bit!

The great thing about Redux is state is whatever you want it to be. As long as youre returning a new state. So this here something we can spin, as at Workpop we've done some nifty things storing a function in the store. Worked well for passing around function callbacks to different components.

So i'm all about creating a standard for this GraphQL Redux interaction.

[tmeasday]

Just to be clear, we aren't proposing using this syntax to interact with the redux store directly. If you want to do that, it seems best to just use another container:

@connect(
  (state, ownProps) => {
    return { id: state.feed.id },
  },
  (dispatch, ownProps) => {
     return { handleError(error) { return dispatch(handleError(error)) },
  });

// here ownProps will come down from the connect container, i.e. include `id` and `handleError`
@graphql((context, ownProps) => {
  // as above
});

class Feed ...

[abhiaiyer91]

Interesting. I'm actually down for this proposal, but I want to poke more.

Why separate state connectors here between UI State and GraphQL State. The beauty of Redux is the ability to colocate this data. Now, it took me a while to get used to, and MAYBE this separation will help people grok it better?

Whats your thinking there?

[tmeasday]

I'd actually be interested in an API like

@connect((ownProps, context, state, dispatch) => ...);

But the argument against it is that it's probably more confusing for people who are following along with our examples to see Redux done in an a different way to what they are used to. Perhaps this API can be exposed for "advanced" users if we like it.

(PS I'm not sure why Redux separates mapStateToProps and mapDispatchToProps, it seems unnecessarily confusing and also slightly restrictive).

[abhiaiyer91]

But the argument against it is that it's probably more confusing for people who are following along with our examples to see Redux done in an a different way to what they are used to.

Yeah this is the thing I'm most worried about. Ideally a uniform experience for Redux users and maybe some power user apis.

Where I'm coming from is, I'd like people to have a jumpstart with easy wins in Apollo Client. Then when they become rockstars, start diving into deeper APIs.

Is that an okay thought?

[tmeasday]

Seems like a good thought on the surface, but I'm not sure which API it pushes us toward?

[abhiaiyer91]

Definitely. In any case, I've exhausted my poking of the design! Let's do it!

[stubailo]

Yeah I was thinking this might actually be easier for Redux devs because they can use the connect they are used to already.

It would be interesting to get the Angular take on this.

[jbaxleyiii]

I've thought about this for a couple days. All in all I'm not a fan of moving state management into the core client.

It's confusing for people to look at the Apollo Client API docs for details of the options etc, and not be looking at the API that they actually call.

I agree that this can be confusing. However, looking at prior art like redux, when libraries choose to be view layer independent, this happens and isn't a bad thing. It helps with triage of issues because the API is a giveaway of where to start looking.

Client integrations must be continually updated to provide the same shape of API as Apollo Client develops.

This was true in the first version of react-apollo, but in the current and the @next version, we pass through all additions to the client API. Also, as long as they are separate projects, there will always be a potential change that means the client can / will be ahead of the view layer.

There's a significant amount of shared logic (for instance, around not re-querying when query arguments don't change) that could be shared between client implementations.

I think an easy (and beneficial) way to solve that is to expose any critical logic as part of the exported methods in the client (not top level)

if the goal here is to unify React/Angular integrations

I don't really think this is a good idea. View layers (and communities) have different conventions and expectations for API design. For instance, we stick the client on the context of react, but in angular, other view layers you would just import the created client for usage. To unify this, we would need to recommend people import the created client for the react integration otherwise we would still have different API's between layers.

So I would say if the goal is to unify view layers, then the view integrations should be part of the client core just like it is with the apollo-server. Then create a universal api and import { reactApollo } from "apollo-client" I'm not a fan of this personally but ¯\_(ツ)_/¯.

[stubailo]

I think everything you're saying makes sense to me. Let's let this cook for a bit more, but my current inclination is that we should just figure out a way to improve our documentation so that people can easily figure out the right syntax for their platform.

[tmeasday]

So I agree (especially after just releasing react-apollo@0.4) that focusing on changing APIs will bring little benefit at this point (and could be a step backwards besides).

I wonder what the appetite is for moving some of the common logic into Apollo Client would be (so perhaps still having a Context object but perhaps not presenting it to the user?)

I think a lot of the code around checking if arguments have changed and keeping track of old results could live there if we think it's something that would generalise across view layers (my intuition is yes).

What do you think @jbaxleyiii? If you think it's a good idea I can make the changes to AC and then send some PRs.

[jbaxleyiii]

@tmeasday I would love that!

[kamilkisiela]

I like the idea. Decorators are just functions so angular2-apollo could use the @graphql inside the @Apollo decorator. The whole logic, which does pretty much the same thing as the react-apollo would be moved to @graphql().

Life cycle hooks would be in charge of controlling the context.

:clap: :+1:

[kamilkisiela]

Docs

and

RxJS implementation

[tmeasday]

See #554 for first pass design.

[stubailo]

@tmeasday do you think this can be closed now? Or is there more work to do?

[tmeasday]

I think so! Will be getting the PR for RA merged soon. Angular folks might want to take a look at the diff: https://github.com/apollostack/react-apollo/pull/211/files

Anything else we want to do can be tracked in separate issues moving forward.