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

Framework: Experiment having a unique Query Component #9386

Closed
wants to merge 11 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@youknowriad
Copy link
Contributor

youknowriad commented Nov 15, 2016

#hackday

My idea was to have one and only way to Query Data, A single QueryComponent (with no need of selectors/actions etc...), but while experimenting I found out that a nice approach to this is to rely on GraphQL. While GraphQL is used most of the time on the server, I thought none was stopping us from using it on the client.

Usage

Let's start by, how to use this API from an external point of view. So to start using this library, you have to wrap your React components with a GraphProvider component, (like react-redux's Provider).

const store = // Redux store
const graph = createGraph( store );
render(
  <GraphProvider graph={ graph }>
    <App /> 
  </GraphProvider>
);

Now that the "graph" is available on the React context, we can use the HoC graphQuery like this:

const MyComponent = ({ results }) => (
   <div>{ JSON.stringify( results ) }</div>
);

export default graphConnect(props => {
  return `
     {
        posts(siteId: ${ props.siteId }) {
          title
          stat( stat: "views" ) 
       }
     }
  `;
})(MyComponent);

And that's it the component will receive the desired data on the results prop.

How does this work?

Well if you're familiar with GraphQL, you already know how this works, for each key of the query, there's a resolver (which is a function that resolves this part of the query), and the graph is basically a tree of resolvers.

Ok we know how to write GraphQL resolvers, but how does this work on Calypso?

A typical resolver in calypso has this shape:

const resolver = (args, { uid }) => {
   // This will ensure the request will be refreshed on each new HoC
   refreshOnUid( store, uid, 'my-resolver', args, () => {
     store.dispatch(actionCreator(args));
  } );

  // Get Data from state
  const state = store.getState();
  const data = mySelector(state, args);
  return data;
}

Ok, but why it's great

  • Because we don't have to worry about how the data is fetched
  • Because we don't need to know about selectors/actions
  • Because the selectors discovery problem is resolved, checkout graphiql https://calypso.live/devdocs/graph?branch=experiment/graph-state
  • Because it forces us to think about caching the data when we write resolvers
  • Because it forces us to type all the data returns by the graph
  • Because refactoring the data is easy, we can easily find which component needs which data
  • Because we can use it for all the state : wheter it's UI data or API data

This is awesome, any drawbacks?

  • The resolver is played each subscribe: This needs profiling but it's not that different from our old approach where connects are played on every subscribe. But YES, we need to think more about the performance aspects.
  • Help think about other drawbacks :)

Experience

For this experience, I updated the PostPerformance Component which displays the last post summary on the stats insights page to use this approach instead of QueryComponents + Selectors.

This component uses two different queries and also needs some UI information, so I thought it was a good example to check different use cases.

cc @aduth

@matticbot

This comment has been minimized.

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Nov 16, 2016

@aduth After our discussion, I added two helpers that ease writing resolvers, refreshOnUid and refreshWhenExpired. The resolvers are shorter and easier to write/understand with those.
So if we want per HoC refresh, we use refreshOnUid and if we want timeout refresh, we use refreshWhenExpired.

@aduth
Copy link
Member

aduth left a comment

A few challenges that I don't see here yet (or failed to understand) are:

  • How would I access nested properties on a collection of nodes? For example, what if I wanted to retrieve a list of posts, and for each post, display the name of the label of the post's type (poor example, I'd hoped to use author's name, but our API is not very REST-y and includes the full author entity already).
  • How do I paginate data? Specifically, how do I know that more paginated data exists? Is this just another resolver that checks against some value we store in Redux state?
  • How do resolvers become aware of mutations (actions dispatched) that occur while I'm using the app?

In some of these cases, particularly pagination, we may want to look to Relay for some guidance on how those problems have been solved there:

http://facebook.github.io/relay/graphql/connections.htm

Ok we know how to write GraphQL resolvers, but how does this work on Calypso?

I don't think we should assume that developers working on Calypso are familiar with GraphQL and writing resolvers. Or at least... I'm not terribly familiar 😄 There will need to be good education around these concepts.


constructor( props, context ) {
super( props, context );
this.graph = props.graph;

This comment has been minimized.

@aduth

aduth Nov 16, 2016

Member

Why not pass the prop directly in getChildContext instead of assigning here?

This comment has been minimized.

@youknowriad

youknowriad Nov 16, 2016

Contributor

Yes, I have not thought a lot when writing this one :) I just copied Redux React Provider. So can't tell why. I think it may be useless :)

This comment has been minimized.

@youknowriad

youknowriad Nov 17, 2016

Contributor

Actually, this is a good practice to avoid updating the context (if someone tries to update the graph prop).

https://facebook.github.io/react/docs/context.html#updating-context

};
};

const createResolver = store => ( args, { uid } ) => {

This comment has been minimized.

@aduth

aduth Nov 16, 2016

Member

I think it is perhaps at this line, but I'm receiving an error when trying to use GraphiQL with the following query:

query {
  posts( siteId: 2916284 ) {
    title
  }
}
{
  "data": {
    "posts": null
  },
  "errors": [
    {
      "message": "Cannot read property 'uid' of undefined",
      "locations": [
        {
          "line": 16,
          "column": 3
        }
      ],
      "path": [
        "posts"
      ]
    }
  ]
}

This comment has been minimized.

@youknowriad

youknowriad Nov 16, 2016

Contributor

I'll investigate, I may have broke something

This comment has been minimized.

@youknowriad

youknowriad Nov 17, 2016

Contributor

This is fixed in 609a6cd

@@ -99,6 +102,17 @@ module.exports = {
}
},

graph: function( context ) {

This comment has been minimized.

@aduth

aduth Nov 16, 2016

Member

This should be included in something the DevDocs section instead, especially to avoid adding unnecessary bloat of GraphiQL to the stats bundle.

This comment has been minimized.

@youknowriad

youknowriad Nov 16, 2016

Contributor

Yeah agree, I've not had the time yet to look at how the DevDocs are built etc... so It was quicker for me to have something showing up for now.

@@ -31,7 +31,7 @@ export default class PaginatedQueryManager extends QueryManager {
}

return PAGINATION_QUERY_KEYS.some( ( key ) => {
return query.hasOwnProperty( key );
return ( {} ).hasOwnProperty.call( query, key );

This comment has been minimized.

@aduth

aduth Nov 16, 2016

Member

What's the issue this was trying to solve?

This comment has been minimized.

@youknowriad

youknowriad Nov 16, 2016

Contributor

This one. http://eslint.org/docs/rules/no-prototype-builtins

I think we should enable this by default. I had a bug because of this. I think GraphQL might be using Object.create


componentWillUnmount() {
this.unsubscribe && this.unsubscribe();
this.context.graph.store.dispatch( clearRequests( this.uid ) );

This comment has been minimized.

@aduth

aduth Nov 16, 2016

Member

Why do we expose the Redux store on the lib/graph module? Why not just access this.context.store directly?

This comment has been minimized.

@youknowriad

youknowriad Nov 16, 2016

Contributor

Because we're not tied to react-redux for this lib. We could imagine using graphConnect without react-redux at all. (SoC)

}

componentWillUnmount() {
this.unsubscribe && this.unsubscribe();

This comment has been minimized.

@aduth

aduth Nov 16, 2016

Member

When is this.unsubscribe falsey?

This comment has been minimized.

@youknowriad

youknowriad Nov 16, 2016

Contributor

never since componentDidMount will be called before, but It seems to me like a best practice to check all extra class attributes before using them. (I do the same for refs).

My logic here is : Here we know it's a React Component Class, so yeah certainly componentDidMount is called before but we think in a more OO way, componentDidMount is not the constructor of our class, so I check first.

But I don't mind droping it

import { requestPostStat } from 'state/stats/posts/actions';
import { refreshWhenExpired } from './utils';

const createResolver = store => ( args ) => {

This comment has been minimized.

@aduth

aduth Nov 16, 2016

Member

Should be consistent with your approach to single argument arrow functions.

store.dispatch( requestPostStat( stat, siteId, postId ) );
} );
const state = store.getState();
const statValue = getPostStat( state, stat, siteId, postId );

This comment has been minimized.

@aduth

aduth Nov 16, 2016

Member

Maybe just return here instead of assigning to variable? Or do you find the variable name helps readability?

This comment has been minimized.

@youknowriad

youknowriad Nov 16, 2016

Contributor

You're right. It's like that because the first version of the resolver was using the variable internally

/**
* External dependencies
*/
importdebounce, now } from 'lodash';

This comment has been minimized.

@aduth

aduth Nov 16, 2016

Member

Why _.now() vs. Date.now() ?

This comment has been minimized.

@youknowriad

youknowriad Nov 16, 2016

Contributor

I don't know, an old habit :)


componentDidMount() {
this.unsubscribe = this.context.graph.store.subscribe( () => {
this.request();

This comment has been minimized.

@aduth

aduth Nov 16, 2016

Member

Can you explain why we trigger this.request() on every dispatch to the Redux store?

This comment has been minimized.

@youknowriad

youknowriad Nov 16, 2016

Contributor

If the state changes, the resolvers need to update their results. That's the same thing as connect I guess.

This answers one of your reviews questions about "resolvers aware of mutations".

}
type Requests {
posts(siteId: Int, query: PostQuery): Boolean

This comment has been minimized.

@aduth

aduth Nov 16, 2016

Member

We might want to start thinking about spacing guidelines for schemas and queries. While this is more conventional in GraphQL, our standards typically have spaces within parentheses.

This comment has been minimized.

@youknowriad

youknowriad Nov 16, 2016

Contributor

Yes agree

@dmsnell

This comment has been minimized.

Copy link
Contributor

dmsnell commented Nov 16, 2016

What is the i in GraphiQL for?

@aduth

This comment has been minimized.

Copy link
Member

aduth commented Nov 16, 2016

I like what I see with GraphQL, and I could get on board with using it in the client to declare data dependencies. Relay and/or GraphQL on the server, on the other hand, appear to be a ways off if even we want to head that direction. Tough concepts like pagination, cache invalidation, and nested property traversals seem to be topics that will cause us some trouble, so are worth exploring now versus later. I see a bit of overlap between here and @dmsnell's #8606, particularly with state.requests, so would be good to start thinking through how/if those will work together.

@aduth

This comment has been minimized.

Copy link
Member

aduth commented Nov 16, 2016

@dmsnell : GraphiQL is Facebook's amazing GraphQL IDE: https://github.com/graphql/graphiql

@dmsnell

This comment has been minimized.

Copy link
Contributor

dmsnell commented Nov 16, 2016

I see a bit of overlap between here and @dmsnell's #8606

I personally see huge overlap; maybe to the extent that these are alternative solutions. it would be great to have a framework pow-wow where a bunch of us could interactively talk through some approaches and finally get something out there.

One of the best pieces of this here is also a good piece in the work @coderkevin is doing in synchrotron: the connect()-like function which requests and fills the props removing the need for selectors. Considering its usefulness I would foresee adding it on top of the API middleware work, though in that case it's an orthogonal system; i.e. that's a further optimization that layers well with the API middleware but not necessary.

The interactive graph query explorer is a strong positive here. I wonder if it actually would resolve our selector discoverability issue or just shift it to the new embedded graph language. Here we have a tool to search for those, but also @aduth just made his own selector discovery tool in DevDocs. We're still left with big questions about naming and organizing data. Is it site(siteId: …) { plans } or plans(siteId: …)? We could have both, but we already have the ability to have both with our selectors if we wanted.

A downside to this approach that I see is that it doesn't help us separate and centralize data policies such as retry-on-network-failure and offline-queuing (or doesn't appear to). Also I wonder if it might add so much complexity that it's not entirely worthwhile.

My approach has always been pitted against something like GraphQL and its ability to reduce queries. My approach is also much simpler and specific to Calypso and dotcom APIs. These are some high-level tradeoffs.

This is a great time to examine different approaches. Let's continue the discussion!

@dmsnell

This comment has been minimized.

Copy link
Contributor

dmsnell commented Nov 16, 2016

doesn't GraphQL require us to explicitly ask for each parameter we use? I think in many places we assume we will get all of the properties of data from state or from a Flux store etc… would we have to rewrite all of these to request each parameter?

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Nov 16, 2016

@aduth

How would I access nested properties on a collection of nodes? For example, what if I wanted to retrieve a list of posts, and for each post, display the name of the label of the post's type (poor example, I'd hoped to use author's name, but our API is not very REST-y and includes the full author entity already).

Actually, I already have an example for that withstatsnested into posts So basically I can write something close to

{
  post( id ) {
    stat( name) 
  }
}

we could easily write an author subresolver to posts that just calls the root user resolver and then we could write

{
  post( id ) {
    author {
      name
    }
  }
}

How do I paginate data? Specifically, how do I know that more paginated data exists? Is this just another resolver that checks against some value we store in Redux state?

Pagination is not so difficult to handle I think. Actually, I already have some pagination since my posts resolver can take any parameter of posts query including pagination. And to check if there are remaining pages, we just add another resolver as you said to extract some value from the store (posts found for example)

How do resolvers become aware of mutations (actions dispatched) that occur while I'm using the app?

Already answered above, they are replayed on each subscribe like react-redux's connect.

I don't think we should assume that developers working on Calypso are familiar with GraphQL and writing resolvers. Or at least... I'm not terribly familiar 😄 There will need to be good education around these concepts.

Yes, but the learning curve for my implentation is not so high. I learned all the concepts and implemented it on one day, but yes we need some education.

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Nov 16, 2016

@dmsnell

I personally see huge overlap; maybe to the extent that these are alternative solutions. it would be great to have a framework pow-wow where a bunch of us could interactively talk through some approaches and finally get something out there.

I agree 👍

We're still left with big questions about naming and organizing data. Is it site(siteId: …) { plans } or plans(siteId: …)? We could have both, but we already have the ability to have both with our selectors if we wanted.

Actually, one of the nice things here is that we don't have to ask ourselvers those questions anymore. We would have both here with the same code , we would use plans( siteId ) if we want to access plans directly ( imagine other parameters that siteId ). and we would use site (siteId) { plans } when we want the site and its plans.

A downside to this approach that I see is that it doesn't help us separate and centralize data policies such as retry-on-network-failure and offline-queuing (or doesn't appear to). Also I wonder if it might add so much complexity that it's not entirely worthwhile.

Actually, the GraphQL is just a tool here, for combining selectors and QueryComponents to simplify getting data to components and make the components more reusable and standalone. But the concers about retry-on-network-failure, offline-queuing etc... are left to actions here (and maybe the API middleware later)

This is a great time to examine different approaches. Let's continue the discussion!

Totally

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Nov 16, 2016

doesn't GraphQL require us to explicitly ask for each parameter we use? I think in many places we assume we will get all of the properties of data from state or from a Flux store etc… would we have to rewrite all of these to request each parameter?

Yes and I think it's a good thing, it's straightforward and will allow us to know exactly which component uses which data, refactoring will be way easier.

@dmsnell

This comment has been minimized.

Copy link
Contributor

dmsnell commented Nov 16, 2016

Yes and I think it's a good thing, it's straightforward and will allow us to know exactly which component uses which data, refactoring will be way easier.

probably a good thing, but a YUUUUGE PR to make the change 😉

@youknowriad youknowriad force-pushed the experiment/graph-state branch from 64f9775 to 5c25fe0 Nov 17, 2016

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Nov 17, 2016

I made some improvements here :

  • Handling query variables to avoid string concatenations
  • Move GraphiQL to devdocs
  • Add some comments to the schema (they show up as descriptions on the discovery feature of GraphiQL)
  • Extract the graph schema to its own file

You can try to write some queries here https://calypso.live/devdocs/graph?branch=experiment/graph-state

@youknowriad youknowriad force-pushed the experiment/graph-state branch from 5c25fe0 to 042c214 Nov 24, 2016

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Nov 24, 2016

@aduth I worked on a more complex use-case with pagination etc... (PostList and PostsNavigation) and it's working properly. I think it's time to do some performance tests. (Maybe I'll need some help on this)

@dmsnell dmsnell self-assigned this Nov 28, 2016

@aduth

This comment has been minimized.

Copy link
Member

aduth commented Jan 19, 2017

A thought that popped into my head today: One of the nice things about Relay.js is how it analyzes every data dependency of the application to decide the fields to be requested from the API. In Calypso 'til now, and as implemented here, we've traditionally just requested the full response payload of any particular endpoint. However, if we could determine the exact fields we use (without degrading the developer experience), we might be able to optimize those requests with a combination of the ?fields parameter and endpoint-specific handling of retrieving only the requested fields. One such endpoint which already handles this well is the sites endpoint, which also happens to be our slowest endpoint in most cases when returning all fields.

Do you have any thoughts on whether this could be achievable? Perhaps as a future improvement?

@dmsnell

This comment has been minimized.

Copy link
Contributor

dmsnell commented Jan 19, 2017

Do you have any thoughts

it's tricky here because we have similar problems on the backend. requesting specific fields is good if we routinely need that same list of fields but it can also thrash caches because instead of requesting the same information a hundred times we might actually be requesting a hundred different shapes of that data each once.

@aduth

This comment has been minimized.

Copy link
Member

aduth commented Jan 19, 2017

it's tricky here because we have similar problems on the backend.

To my knowledge we're not caching the response payloads in their entirety, but rather rely on internal object caching of individual WordPress getter APIs, e.g.

Which would seem to be fine even if we vary fields requested.

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Jan 20, 2017

@aduth From a technical POV, I'm not sure this is possible using the GraphQL library, because when we hit the resolver responsible for triggering the API call, we don't receive the "fields" requested as an argument or something. If we use our custom Query Engine, we could enhance it to provide this information, but I don't know, I feel we should stay as compatible as we can to the official GraphQL implementation (even if this argument can be optional).

@dmsnell

This comment has been minimized.

Copy link
Contributor

dmsnell commented Jan 20, 2017

I'm not sure this is possible using the GraphQL library

there must be something I'm mistaking. isn't this one of the central concepts of GraphQL? that the queries determine the shape (i.e. the fields) of the data being requested and then that graph of requests is minimized/optimized for the final retrieval?

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Jan 23, 2017

isn't this one of the central concepts of GraphQL?

@dmsnell Yeah it is, but this means when you write a query { post( id: 10 ) { id, title } } the graphql library will return only the id and the title attributes as a response for this query, so if GraphQL is server side, our network response will contain only these attributes. But that does not mean the post resolver didn't return more than that, the post resolver in this case will receive only the args between parens ({ id: 10 }) and will use this to return the post object (and any other subresolver).
The GraphQL library calling the resolver, will filter the value returned from the resolver to match the queried fields, thus, it will extract only the id, and title from the post object.

In our case the network query is triggered inside the resolver, there's no way to tell which subfields are being requested at that moment.

I hope this answers the question

@nb

This comment has been minimized.

Copy link
Member

nb commented Feb 6, 2017

I tried the branch and writing GraphQL queries is more fun than messing with selectors :)

Outside of the performance concerns (it doesn’t look somebody has looked into that), the query felt a bit disconnected from the component itself. What do you think of making the query a static property of the component class?

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Feb 7, 2017

@nb Thanks for testing this branch, I really appreciate it.

Outside of the performance concerns (it doesn’t look somebody has looked into that)

I did some tests myself, And if I came up with a "light graphql executor" used only in production which skips validation and parsing of the request. I think we this, we are really close to the performances we have with react-redux. Though more persons make performance tests would be awesome.

What do you think of making the query a static property of the component class?

You mean something like:

MyComponent.query = props => `
  {
    ...
  }
`;


MyComponent.queryVariables = props => (
  {
    var1: props.prop1
  }
);

export default query( MyComponent );

My feeling about this is that it can achieve the same behaviour, but I don't know I kind of prefer the mapPropsToQuery and mapPropsToQueryVariables as arguments to the HoC (same as react-redux). This allows the separation between the "Presentational component" and the "Container component", (Though, I agree is not a pattern we use that much in Calypso).

@dmsnell

This comment has been minimized.

Copy link
Contributor

dmsnell commented Feb 7, 2017

My feeling about this is that it can achieve the same behaviour, but I don't know I kind of prefer the mapPropsToQuery and mapPropsToQueryVariables as arguments to the HoC (same as react-redux). This allows the separation between the "Presentational component" and the "Container component", (Though, I agree is not a pattern we use that much in Calypso).

my personal idea is some kind of hybrid. we already use connect() and I think we could build our own connect wrapper that includes the Redux API with the additional data request description

@matticbot matticbot added the [Size] XL label Mar 30, 2017

@dmsnell dmsnell removed their assignment Apr 8, 2017

@dmsnell

This comment has been minimized.

Copy link
Contributor

dmsnell commented Apr 18, 2017

@youknowriad should we go ahead and close this out? it's now really old and also it's pretty big so I'm thinking that a new push on this would almost require starting over anyway.

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Apr 18, 2017

Yep, closing for now! I still think this is valuable. I've not been pushing on this because I think this is a big change and maybe we should reduxify everything before introducing a big change like this.

And also, I've extracted the work done here into a library https://github.com/youknowriad/react-graphql-redux. It should be way easier to reuse later. I've been using the library in a personal project and I really appreciate the DevX.

@gziolo gziolo deleted the experiment/graph-state branch Apr 18, 2017

@jasonbahl

This comment has been minimized.

Copy link

jasonbahl commented Dec 4, 2017

Ya'll should check out Apollo 2.0, and Apollo Link. You could start incrementally incorporating GraphQL over time and start seeing great developer experience advantages.

Would be super awesome to see GraphQL implemented in an open source project as large/prominent as Calypso

@nb

This comment has been minimized.

Copy link
Member

nb commented Dec 4, 2017

Thanks, @jasonbahl, we will definitely have a look.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment