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

Data Module: Expose State using a GraphQL API #4083

Closed
wants to merge 1 commit into from

Conversation

youknowriad
Copy link
Contributor

@youknowriad youknowriad commented Dec 19, 2017

This is a POC to trigger a discussion of whether we should continue this way or not. Alternative to #4105 closes #2473

So the idea here is that a module that wants to expose a specific part of its state registers a GraphQL schema describing the exposed data and a GraphQL resolver to fetch this data from the state. ( see editor/state/data.js for an example of a (schema, resolver) pair.

The data module exposes a registerSchema function to register the module's schema/resolver pair.

And a query Higher Order Component is also exposed to fetch data like so:

import gql from 'graphql-tag';
import { query } from '@wordpress/data';
const MY_QUERY = gql`query { editor { post { title } } }`;
const MyComponentWithData = query( MY_QUERY )( props => 
  <div>{ get( props.data, 'editor.post.title', 'no title' ) }</div>
);

@youknowriad youknowriad added the Framework Issues related to broader framework topics, especially as it relates to javascript label Dec 19, 2017
@youknowriad youknowriad self-assigned this Dec 19, 2017
@youknowriad youknowriad requested review from mcsf, gziolo and aduth December 19, 2017 11:41
@youknowriad
Copy link
Contributor Author

cc @atimmer

@youknowriad youknowriad force-pushed the try/graphql-data-api branch 3 times, most recently from ebea8f3 to 26935d7 Compare December 19, 2017 13:49
*/
import { Component } from '@wordpress/element';

const createQueryHigherOrderComponent = ( client ) => ( mapPropsToQuery, mapPropsToVariables = () => ( {} ) ) => ( WrappedComponent ) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would these more accurately be mapQueryToProps and mapVariablesToProps ?

Or am I wrong in thinking that these functions would take values from the query result and map them to props for the component?

Copy link
Contributor Author

@youknowriad youknowriad Dec 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, they are correctly names, these are useful if you want to create custom queries based on props.

I can see something like mapResultToProp being usefull as well. It could be a third optional callback.

Edit In a GraphQL API, this is less important though because you already ask for the data you want, it could be convenient though for "undefined" checks and stuff like that.

@aduth
Copy link
Member

aduth commented Dec 19, 2017

In your example, wouldn't you want a mapping function from the query result to the props to pass to the underlying component, like connect's mapStateToProps ? To make the component less dependent on the schema shape?

@youknowriad
Copy link
Contributor Author

There's a bug in the production build npm run build but not sure why. It looks like it's caused by the webpack UglifyJsPlugin.

@@ -0,0 +1,66 @@
/**
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor, it might be located next to other HOCs inside @wordpress/components.

@gziolo
Copy link
Member

gziolo commented Dec 20, 2017

get( props.data, 'editor.post.title', 'no title' )

It would be nice to be able to use it as props.title inside the component. I assumed get is from Lodash, which makes it look like an inlined selector in the current shape. Which leads me to the conclusion that you need to get the same item twice. First in the query resolver and then when applying query HOC.

I know all the benefits of GraphQL, how it can scale well, provides a unified interface and helps to limit the number of requests. I can imagine you even would be able to wrap withApiData with query to unify experience. My biggest concern is that the learning curve might be higher than in case of selectors. I'm especially worried about people coming with jQuery + Backbone background and now suddenly they need to learn ES.Next, React, Redux, GraphQL, etc :)

Do you see benefits of having GraphQL in the core on top of selectors and have the latter exposed for plugin developers? I'm just trying to find a way that satisfies needs of all parties involved here.

@youknowriad
Copy link
Contributor Author

Do you see benefits of having GraphQL in the core on top of selectors and have the latter exposed for plugin developers? I'm just trying to find a way that satisfies needs of all parties involved here.

No, it will be useless in this case. The advantage of GraphQL is being this discoverable and easy to use API. It certains adds learning curve to people creating data (module creators and advanced plugins that want to use the system to expose their own data), but from a consumer's perspective, which I think is the majority of plugin authors and developpers, its API is even simpler to grasp than having to learn and also find every available selector.

this.cancelRequest();
const query = client.query( { query: this.query, variables: this.variables } );
const observer = query.subscribe( ( results ) => {
this.setState( results );
Copy link
Member

@atimmer atimmer Dec 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In your example in the pull request description, the data is accessed using props.data, but in the higher order component, you only set the state. How does this work?

Copy link
Contributor Author

@youknowriad youknowriad Dec 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, actually a GraphQL result is always like this { data, errors } so we'll end up with these two props.

For convenience, we could add an optional callback to "polish" the props like suggested by @aduth. mapResultsToProps

@youknowriad
Copy link
Contributor Author

Closing this as I think we should go with the selectors API

@youknowriad youknowriad deleted the try/graphql-data-api branch January 15, 2018 09:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Framework Issues related to broader framework topics, especially as it relates to javascript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

A way for plugin developers to access the editor state?
4 participants