Skip to content
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

interaction with domain state and redux #98

Closed
abhiaiyer91 opened this issue Apr 12, 2016 · 14 comments
Closed

interaction with domain state and redux #98

abhiaiyer91 opened this issue Apr 12, 2016 · 14 comments

Comments

@abhiaiyer91
Copy link
Contributor

I have a couple questions about the Apollo Client store implementation

I see in the example that we have one reducer called apollo. What state does this represent? The current query? The thing with ambitious projects like this is I feel that we should be thinking long term. Having all state under one reducer seems like a bad idea and hard to manage going forward. Imagine mapping state to props over a huge reducer whos shape may be nested etc.

you may end up with something like

function mapStateToProps(state) {
   return {
     job: state.apolloReucer.job.title
    foo: state.apolloReducer.foo.bar.baz
  };
}

Is there a way we can represent each query has a slice of our state. Where we somehow generate multiple reducers to manage domain state from different queries?

Sorry if this is naive in approach, just going off the docs at this point!

@stubailo
Copy link
Contributor

The apollo client itself manages both the reducer and mapping state to props - so rather than using redux-react, you would use watchQuery with a custom Apollo container. Check out this article:

https://medium.com/apollo-stack/building-a-graphql-store-from-first-principles-413e19ea65b4

Also, if you have read about Relay, it uses a similar store shape, and it does the same management for you under the hood.

The point is, the store needs to be a normalized cache of result objects, rather than something that keeps the result of each query independently. Right now, it doesn't bring as much benefit, but once you start dealing with pagination, optimistic UI, data preloading, refetching, etc - you basically need a normalized store to take advantage of GraphQL.

In short, we don't expect people to be using the apollo part of the redux store directly, but rather with some provided helpers. Here's a sketch of a React container I wrote for a demo:

const Topic = ({ data, loading, loginToken }) => (
  <div>
    { loading ? 'Loading...' : <TopicLoaded data={data} loginToken={loginToken} /> }
  </div>
);

const TopicWithData = createContainer({
  getQuery: ({ params, loginToken }) => {
    const maybeLoginToken = loginToken ? `(token: "${loginToken}")` : '';

    return `
      {
        root${maybeLoginToken} {
          oneTopic(id: "${params.id}") {
            id
            category_id
            title
            posts {
              pages {
                posts {
                  username
                  score
                  cooked
                }
              }
            }
          }
        }
      }
    `;
  },
}, Topic);

(that's not the final API, just a sketch)

@jbaxleyiii
Copy link
Contributor

@abhiaiyer91 one thing about the current store implementation, is there isn't a 1:1 query: data mapping. We normalize the data so using mapStateToProps like that wouldn't be possible.

We are working on an easy integration with both redux and react. I'm actually working on the redux api today! Ideally something like this:

import { readFromStore } from 'apollo-client'

function mapStateToProps(state) {
   return {
     job: readFromStore(/.+Job/)
    foo: readFromStore(/.+Foo/)
  };
}

This is a similar proposal syntax to #26.

That being said, I'd also like a clean way to say treat this query as a collection of data, normalize it, and let me get the data back out easily. I don't love the regex method so still exploring ideas!

Sorry if this is naive in approach, just going off the docs at this point!

It's worth mentioning the library is still undergoing some API changes (for instance #97) and also doesn't export all of the needed methods yet (for instance createNetworkInteface) but should soon

@stubailo
Copy link
Contributor

@jbaxleyiii have you considered using readFragmentFromStore instead of the regex? Then you can just pass the root ID and a query to get data in the shape you want.

It sounds like writing a React container and demonstrating how I was thinking the data would be used should be a critical priority.

@jbaxleyiii
Copy link
Contributor

@stubailo I was just playing around with that 👍. I'm working on redux and react implementations today as we continue moving our app over. I think I get what you are thinking with your example and have created a couple implementations to try it out. Its working great so far.

@stubailo
Copy link
Contributor

Here's my current naive implementation:

import React from 'react';
import { ApolloClient } from 'apollo-client';
import { createNetworkInterface } from 'apollo-client/lib/src/networkInterface';
import PureRenderMixin from 'react-addons-pure-render-mixin';

export const client = new ApolloClient();

export function createContainer(options = {}, Component) {
  const {
    getQuery,
    pure = true
  } = options;

  if (! client) {
    throw new Error('need to pass in apollo client');
  }

  const mixins = [];
  if (pure) {
    mixins.push(PureRenderMixin);
  }

  /* eslint-disable react/prefer-es6-class */
  return React.createClass({
    displayName: 'ApolloContainer',
    mixins,
    getInitialState() {
      return {
        loading: true,
      };
    },
    componentDidMount() {
      this.runMyQuery(this.props);
    },
    componentWillReceiveProps(nextProps) {
      this.queryHandle.stop();

      this.runMyQuery(nextProps);
    },

    runMyQuery(props) {
      this.setState({
        loading: true,
        data: null,
      });

      this.queryHandle = client.watchQuery({
        query: getQuery(props),
      });

      this.queryHandle.onResult((error, data) => {
        if (error) {
          throw error;
        }

        this.setState({
          data,
          loading: false,
        });
      });
    },

    componentWillUnmount() {
      this.queryHandle.stop();
    },
    render() {
      return <Component {...this.props} {...this.state}/>;
    }
  });
}

@abhiaiyer91
Copy link
Contributor Author

See now this makes more sense. In my mind I envision this playing out something like:

Server data:
ApolloClient -> watchQuery -> container component -> render
Client Data
Store -> ReactRedux -> container component -> render
Then theres a mix client state to server state back to client
Store -> stateFromClient -> watchQuery -> container component -> render

From there you use ActionCreators to as the public API to update Client State / Server State.

Whats the API look for calling a mutation in this world? Are we still using thunk writing functions that under the hood call Apollo methods?

@stubailo
Copy link
Contributor

No right now you call client.mutate and that does the right Redux actions for you. Redux is more of an underlying store and debug log than an end-user API in this model.

Ideally the React container for Apollo will also let you do the regular Redux mapStateToProps, so you can just use one container to get a graphql query, and your client-side data.

@abhiaiyer91
Copy link
Contributor Author

Dont you think encapsulating client.mutate into a pattern will help end-users consolidate their business logic/ state update patterns?

I like that container can mapState, much like a Meteor createContainer + connect does. I'm sold on the data fetching part of this.

Still not entirely sold on the update api right now

@stubailo
Copy link
Contributor

Yeah, the mutation API is the weakest part at the moment. Once we start thinking about optimistic UI and how data refetching after mutations works, it will probably get better.

One that that would be great is if mutations and redux actions could be fused together, so you could update your server and client in one step.

I like that container can mapState, much like a Meteor createContainer + connect does. I'm sold on the data fetching part of this.

Great! Yeah I think not needing to stack up two containers will be great. I think having a one-stop-shop for data is going to be really cool.

@abhiaiyer91
Copy link
Contributor Author

Awesome! I'm going to start using Apollo for an internal tool. Will probably have my own opinions about some of the API design once im in the thick of it. Ill report back on the issues any ideas I have

@stubailo
Copy link
Contributor

Dude amazing!

@stubailo
Copy link
Contributor

Gonna close this issue, but I listed it in the README as a good conversation to look at: 6de2414

@AndrewHenderson
Copy link

AndrewHenderson commented May 10, 2017

@stubailo I'm attempting to query the current user's User data on the root React.Component of my application and provide the data to the children which are React Router routes.

export class App extends Component {

  render() {
    return (
      <div>
        <Navbar { ...this.props.data.user } />
        { React.cloneElement(this.props.children, { user: this.props.data.user }) }
        <TimelineContainer />
      </div>
    );
  }
}

const ViewerQuery = gql`
  query userQuery($personId: String) {
    user(personId: $personId) {
      firstName
      lastName
      personId
      username
    }
  }
`;

export default graphql(ViewerQuery, {
  options: ownProps => ({
    variables: {
      personId: 2
    }
  })
})(withApollo(connect()(App)));

I don't like using React.cloneElement. Ideally, I'd use readQuery however, the data has yet to be returned by the time the children mount.

What is the recommended approach in a case such as this?

A coworker recommended I use the same query in the children since Apollo should see that it's got all the necessary data and therefore not send our another GraphQL request.

Thanks in advance!

@helfer
Copy link
Contributor

helfer commented May 11, 2017

@AndrewHenderson If your children mount before the data is completely fetched, the children will fire their own queries. Is there a reason you don't want to stop the children from rendering while the data hasn't loaded yet? Do they have something useful to display without data? If not, you can check props.data.loading.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants