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

Framework: Declarative Data Dependencies #13103

Closed
wants to merge 3 commits into from

Conversation

samouri
Copy link
Contributor

@samouri samouri commented Apr 13, 2017

In Calypso, we know that we want declarative dependencies for our ui components/blocks. The big question is how?

My proposal here is to create a new HoC function, needs, that takes the place of connect, query components, and selectors. Under the hood, needs does a few things:

  1. wraps component in a standard React Component that contains all the usual lifecycle hooks
  2. the component issues necessary requests for data on both componentWillMount and componentWillReceiveProps
  3. takes the fetched data from the redux tree once it is available and hands it to its children

API

needs(list of need objects, [mapStateToDispatch])

This composes all of the specified needs into a single HoC that can be used to wrap a component class. It issues any necessary requests on componentWillMount and componentWillReceiveProps.

Arguments
  • [need objects] (array): represents a list of needs.

  • [mapDispatchToProps(dispatch, [ownProps]): dispatchProps] (Object|Function): behaves the same way as redux-connect's mapDispatchToProps docs

Need Object

A need is any object that contains these two functions:

mapStateToProps( state, [ownProps] ):stateProps (Function).

This behaves the same way as connect's first parameter and under the hood gets directly passed to connect.

mapStateToRequestActions( state, [ownProps] ): requestActions (Function).

This is one of the unique bits of this proposal and is a pretty good corollary to query component. Based on state and ownProps, it should figure out what requests need to be made and then return an array of actions that will be dispatched (instead of directly dispatching them).

@matticbot
Copy link
Contributor

@matticbot matticbot added the [Size] M Medium sized issue label Apr 13, 2017
@samouri samouri force-pushed the try/framwork/declarative-needs branch from 4a65917 to 44960a7 Compare April 13, 2017 22:28
@matticbot matticbot added [Size] L Large sized issue and removed [Size] M Medium sized issue labels Apr 13, 2017
@dmsnell dmsnell requested a review from youknowriad April 18, 2017 05:52
@youknowriad
Copy link
Contributor

Nice and simple proposal. It has the advantage of being easy to understand and use. However, I think this is valuable only if we can split big chunks of selectors and actions creators into one need. But I'm afraid that in most cases we'll end up with a single (or two) selector and a single or two actions creators in each "need". Please correct if I'm wrong.

I still feel the best way to declaratively declare data dependencies is using GraphQL or similar and maybe use the library https://github.com/youknowriad/react-graphql-redux and my initial PR about this is #9386

I've not been pushing this way because we have some technical debts (reduxifying, sites-list, ...) we should tackle first before introducing a big change. (Maybe it's time to rethink about it, since we're getting there's an ongoing sites-list dropping project)

@samouri
Copy link
Contributor Author

samouri commented Apr 18, 2017

However, I think this is valuable only if we can split big chunks of selectors and actions creators into one need.

Can you explain what you mean there? What is the problem with having one need per logical thing?

@youknowriad
Copy link
Contributor

Maybe I'm wrong but what I mean is if one "need" like 'readerSite' uses a unique selector (and a unique query), The boilerplate abstracted with this implementation maybe too small and we'll find our selves writing a "need" for each specific selector?

@samouri
Copy link
Contributor Author

samouri commented Apr 18, 2017

writing a "need" for each specific selector?

Does GraphQL allow you to get around that? The way I understood it, every need (query) in GraphQL needs a resolver.

@youknowriad
Copy link
Contributor

youknowriad commented Apr 18, 2017

@samouri Right! but we can use multiple/different selectors in the same resolver depending on the params of our query. They way I understand the needs here, is that we'll have different needs with the same query but different selectors.

Edit:

So basically

  • GraphQL: one resolver for a query component
  • Needs: multiple needs for a query component

@samouri
Copy link
Contributor Author

samouri commented Apr 18, 2017

Oh, I think I see the issue. Let me know if I don't understand this right:

Example Problem: In Reader we have a selector for both get-reader-tags and get-reader-followed-tags. The latter is a subset of the former. They both depend on the same request for data.

If we wanted to uniquely specify some components as needing one and others as needing the other, how would we do it using this approach?

Good question :)

Does GraphQL have a solution to this problem of 'selecting' derived/computed data?

My first two instincts are:

  1. allow needs to also take in a list of selectors in addition to its list of needs. (needs would be for data objects, selectors only for derived ones)
  2. plop all potential sub-selections in the original need

@youknowriad
Copy link
Contributor

The fact that GraphQL is a tree makes this easy to solve you could just write a selector like this pseudo-code:

const readerResolver = () => {
   triggerQuery();

  return {
    tags: () => getReaderTags(),
    followedTags: () => getReaderFollowedTags()
} 

Maybe make the needs configurable: readerData( 'tags' ) instead of readerTags or something like that.

@samouri
Copy link
Contributor Author

samouri commented Apr 18, 2017

Maybe make the needs configurable: readerData( 'tags' ) instead of readerTags or something like that.

I don't know if it should get as general as readerData, because then a single need would be responsible for many server-side datatypes whereas I think the ideal would be 1 need per kind of fetch.

I love this idea for solving the selectors issue!

needs( readerTags( 'followed' ) )( TagSidebar )

// vs

needs( readerTags( 'all' ) )( TagSidebar )

@matticbot matticbot added [Size] XL Probably needs to be broken down into multiple smaller issues and removed [Size] L Large sized issue labels Apr 19, 2017
@samouri samouri force-pushed the try/framwork/declarative-needs branch from 214e50e to e1ea251 Compare May 4, 2017 17:24
@samouri samouri force-pushed the try/framwork/declarative-needs branch 6 times, most recently from 6238d90 to afae10d Compare May 25, 2017 19:10
@samouri samouri force-pushed the try/framwork/declarative-needs branch from 48c57f8 to b02acc0 Compare May 25, 2017 19:41
@samouri
Copy link
Contributor Author

samouri commented May 25, 2017

Some things i'm still thinking about:

  1. is there any common ground to be found between needs and selectors to have them build off of each other? Are there any cases where you would want to use a selector instead of a need or does a need totally remove the need for selectors? We still want memoization for computed properties
  2. how can this integrate more tightly to the data-layer?
  3. Is this a good place to add in a concept of data-freshness? I can easily imagine a needs object accepting a property like freshness: seconds so that each wrapped component could have its own freshness requirements.

@AlanFoster
Copy link

Just as an external opinion; I do not actively contribute to calypso, but I think trying to make use of GraphQL as a query language would be a good choice for specifying data requirements on components.

For context - I am currently developing a larger react-native application that makes use of Redux. I have ran into the exact same problem were I want data-driven widgets, and a declarative API definitely seems like the right approach to offer this functionality.

To implement this currently, I have rolled our own declarative API, that is even more generic than the proposed solution within this PR. It also favours making use of connectAdvanced rather than the connect approach within this PR, and offers an API similar to connect

The declarative API is agnostic of API calls specifically, and it just needs to be given a promise that it will await, which should succeed or fail.

As an example of its API:

  // The function call itself only needs to return a promise which can succeed/fail
  const someDataDrivenApi = function (...args..) { 
    return fetch('...');
  }

 --

  const dataRequirements = function(state, ownProps) {
    return {
      repositories: {
        call: someDataDrivenAPI,
        args: { type: ownProps.type }
      },
      someOtherThing: {
        call: someDataDrivenAPI,
        args: { dateRange: ownProps.dateRange, foo: true }
      }
    };
  };

   // Specify the props for our Component, with full access to the original props + redux state tree, as well as the data driven results
  const mapDataToProps = function(state, ownProps, { data, isLoading, fetchMore }) {
    return {
       ...data,
       otherThing: selectors.fooSelector(state),
       etc
     };
  };

  const DataDrivenComponent = dataDriven(dataRequirements, mapDataToProps)(
    SomeComponent
  );

The data driven component handles the caching mechanism for us within our redux tree, so various components that are using the same data in different places only need to request data once. Specifically under the hood maps the call + specific args request to a response, and checks if it has previously searched for it. This is in contrast to this PR's suggestion of letting the user decide when it has data available currently. This approach means we can abstract such data decisions entirely from the user. The redux tree also stores the request time, and the current status of the request - such as Pending, Success, Failure, etc. The request time can be used to implement a LRU cache to get rid of stale information, or refresh it if required.


The approach I have described above works well for us currently. But I feel like using an Apollo client with a GraphQL client would make things even better - and take it a step further.

Our current data driven components don't need server side mutations - but choosing GraphQL as a language might be useful in this scenario too.

TL:DR; We've taken a similar approach for a declarative API which allows for components to specify their data requirements, but would really like to make use of Apollo + client side resolvers, or at the very least GraphQL as the query language for specifying data requirements ¯_(ツ)_/¯

@ockham
Copy link
Contributor

ockham commented Oct 23, 2017

Drive-by, and JTFR -- I'd love more decalarative ways of data fetching, but please let's try to make sure they work with SSR 🙂

@apeatling
Copy link
Member

This one seems to have stalled. I'm going to close, please reopen if there's more to commit or we can meaningfully unblock this.

@apeatling apeatling closed this Nov 7, 2017
@apeatling apeatling deleted the try/framwork/declarative-needs branch November 8, 2017 03:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Framework [Size] XL Probably needs to be broken down into multiple smaller issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants