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

Idea: Factor out a graphQLSelector to be used independently #244

Closed
stubailo opened this issue May 25, 2016 · 13 comments
Closed

Idea: Factor out a graphQLSelector to be used independently #244

stubailo opened this issue May 25, 2016 · 13 comments
Milestone

Comments

@stubailo
Copy link
Contributor

stubailo commented May 25, 2016

@gaearon has been talking a ton about a new-ish concept for Redux called "selectors" - basically the idea that your Redux store shape can be an implementation detail of the store logic, and then you use custom functions as the actual public API to get data out.

readQueryFromStore is basically exactly this for Apollo - it's a selector that takes a GraphQL query and generates that result from the normalized store format. Now that there's a name for this concept and some great recommendations for how to use selectors, we should make sure that taking this approach with Apollo Client makes sense. We might also want to rename our internal functions to map onto this concept so that people familiar with Redux might feel more at home.

This can be as simple as exporting the selector properly, and teaching people how to use that in vanilla Redux connect or their reducers to do custom GraphQL stuff.

@abhiaiyer91 want to take a stab at the design? Since the ApolloClient takes some options that affect how denormalization/selecting might work, we might want this to be a function on client, something like:

// Question: where to get `client` from? It lives on context, but we can't access that here...
mapStateToProps: (state) => ({
  graphQLData: client.selector(state.apollo.data, {
    query: gql`...`,
    variables: {},
  })
})

With a little bit of cleverness, this API could be extended to work for fetching any appropriately normalized Redux data with a GraphQL query.

@stubailo stubailo added the idea label May 25, 2016
@abhiaiyer91
Copy link
Contributor

I'm all about this. I'm in.

@stubailo
Copy link
Contributor Author

OK I have a few important questions:

  1. How do we get a reference to the client from here? Importing seems wrong because we have ApolloProvider specifically so that we don't need to import. But the client instance could have important config options or plugins we would want to use in the selector.
  2. What options might this need to take? For example, how would the user find out if not all of the data they asked for was in the store? One solution is to have returnPartialData, and throw an error if some of the requested data doesn't exist.
  3. If we want to do some of the caching that watchQuery now does as of Do not broadcast queries if Apollo state has not changed and if the previousState actually has anything inside of it. #226, where do we keep that state? How does reselect do it?

@abhiaiyer91
Copy link
Contributor

I'll get started on this ASAP.

@abhiaiyer91
Copy link
Contributor

I have a question about this one. Why does this have to be a method off the client itself, this should be a stateless function that handles the computation no? Ideally a selector that given inputs outputs normalized data?

@abhiaiyer91
Copy link
Contributor

Also what do we gain here? I guess it's hard to see how people would use this when they can just mapqueries?

@abhiaiyer91
Copy link
Contributor

Or actually we could replace mapqueries entirely. Collocate your graphql state with client state

@abhiaiyer91
Copy link
Contributor

abhiaiyer91 commented Jun 1, 2016

Alright, so I got this going...

function getData(state, query) {
  try {
    return readQueryFromStore({store: state.apollo.data, query});
  } catch (e) {
  }
}


function mapStateToProps(state) {
  return {
    graphqlData: getDataSelector(state, gql`
          {
            counts {
              _id
              count
            },
          }
        `)
  };
}

so it takes a couple runs of mapState to get the result. Which is chillin. Just sucks we have to trycatch! Also this example is pretty useless because it is mapQueriesToProps that is starting the watchQuery.

@abhiaiyer91
Copy link
Contributor

abhiaiyer91 commented Jun 1, 2016

I think next to do is refactor the "selectors" we already have to be inline with something like common redux patterns.

Still need to explore 2. of your question. As for 3. i think we can use reselect itself?

@davidwoody
Copy link
Contributor

@abhiaiyer91 @stubailo being able to utilize mapStateToProps for actually reading data out of the store makes a lot of sense and feels like it's more approachable / understandable for folks who are familiar with Redux.

Also this example is pretty useless because it is mapQueriesToProps that is starting the watchQuery.

I think mapQueriesToProps still could make sense, but what the container component does could be simplified to:

  1. Calling client.query or client.watchQuery to fetch the data from the server.
  2. Mapping the queries to props (re-rendering whenever ownProps or mapQueriesToProps return value changes)

That would result in something like:

const mapStateToProps = (state, ownProps) => ({
  todos: readQueryFromStore(state.apollo.data, ownProps.todosQuery),
});

const mapQueriesToProps = (state, ownProps) => ({
  todosQuery: {
    query: gql`...`
    variables: { ... }
  }
});

export default compose(
  apolloConnect(mapQueriesToProps),
  reduxConnect(mapStateToProps),
)(TodoList);

As far as the question in 2.

What options might this need to take? For example, how would the user find out if not all of the data they asked for was in the store? One solution is to have returnPartialData, and throw an error if some of the requested data doesn't exist.

This could just be another "selector" into the Apollo state.

const mapStateToProps = (state, ownProps) => ({
  todos: readQueryFromStore(state.apollo.data, ownProps.todosQuery),
  loading: isFetchingQueryFromServer(state.apollo.queries, ownProps.todosQuery)
});

If we want to do some of the caching that watchQuery now does as of #226, where do we keep that state? How does reselect do it?

Can that state be kept in redux and provide similar "selector" methods to access the state?

@abhiaiyer91
Copy link
Contributor

abhiaiyer91 commented Jun 6, 2016

@davidwoody But if mapQueriesToProps already maps todos and its data to props, why use mapStateToProps to fetch the same data? Or are you saying that we can use mapStateToProps primarily for selectors that take the data of the apollo store and some other stuff to compute some new type of state? Without that case, mapStateToProps and mapQueries doing the same thing is overkill?

EDIT: i see what youre saying, let me try to document the couple flows you would have has a consumer

@davidwoody
Copy link
Contributor

@abhiaiyer91 This may or may not be helpful, but this is sort of how I'm thinking about apolloConnect in my example working for mapQueriesToProps:

const apolloConnect = (mapQueriesToProps) => (component) => {
  class Apollo extends Component {
    constructor(props, context) {
      super(props, context);
      this.store = context.store;
      this.client = context.client;
      this.state = Object.assign({}, this.store.getState());
    }

    componentWillMount() {
      this.unsubscribeFromStore = this.store.subscribe(() => {
        this.setState(this.store.getState());
      });
      this.fetchQueries();
    }

    shouldComponentUpdate(nextProps, nextState) {
      let shouldUpdate = true;
      const nextQueryHandles = mapQueriesToProps(nextState, nextProps);
      const queryHandlesChanged = !_.isEqual(nextQueryHandles, this.queryHandles);
      const propsChanged = !_.isEqual(nextProps, this.props);

      // only re-render if props or queryHandles changed
      if (!propsChanged && !queryHandlesChanged) {
        shouldUpdate = false;
      }

      return shouldUpdate;
    }

    componentWillUpdate() {
      this.fetchQueries();
    }

    componentWillUnmount() {
      this.unsubscribeFromStore();
    }

    fetchQueries() {
      const { client, store, props } = this;
      const queryHandles = mapQueriesToProps(store.getState(), props);

      Object.keys(queryHandles).forEach((queryHandleKey) => {
        // tells Apollo to fetch data from server
        client.query(queryHandles[queryHandleKey]);
      });

      this.queryHandles = queryHandles;
    }

    render() {
      // passes own props and queries to component
      const props = Object.assign({}, this.props, this.queryHandles);
      return createElement(component, props);
    }
  }

  Apollo.contextTypes = {
    store: PropTypes.object.isRequired,
    client: PropTypes.object.isRequired,
  };

  return Apollo;
};

@deoqc
Copy link

deoqc commented Aug 16, 2016

I use global unique id's and nodes (actually using graphql-relay helpers for this). It may have some advantages, like automatically updating mutations (see the dataIdFromObject here), or some more discussion here.


The thing is, ideally this selectors could support get data from store by its id, something like:

const mapStateToProps = (state, ownProps) => ({
  todo: getFromStoreId(ownProps.id),
})

The default behaviour would be return the entire denormalized data, and may pass an optional query second arg to specify it.

@stubailo
Copy link
Contributor Author

Well guys I wanted to have some fun, so I made a ton of progress in this direction:

https://github.com/stubailo/graphql-anywhere

It's a totally standalone library that factors out all of apollo's query execution functionality, and even removes the need for a custom store format.

Hope to refactor AC to use this soon, and perhaps make a complementary normalization library - like normalizr, but one that takes advantage of GraphQL's structure.

@stubailo stubailo added this to the New API/Refactor milestone Sep 29, 2016
jbaxleyiii pushed a commit that referenced this issue Oct 17, 2017
jbaxleyiii pushed a commit that referenced this issue Oct 18, 2017
Only send request cookie rather than full headers
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 2, 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

4 participants