silverstripe / silverstripe-admin

Silverstripe Admin Component
BSD 3-Clause "New" or "Revised" License
25 stars 92 forks source link

SPIKE Passing data into a React GridField #537

Open chillu opened 6 years ago

chillu commented 6 years ago

Parent issue: https://github.com/silverstripe/silverstripe-admin/issues/463

RFC: https://github.com/silverstripe/silverstripe-admin/issues/556 POC: https://github.com/open-sausages/react-gridfield-poc

Description

In 3.x, data is passed into a GridField through a DataList or ArrayList. The underlying APIs are the same between fetching data for CMS editing and exposing them in your own SilverStripe templates.

In 5.x, we want tabular view components to be powered by GraphQL. This presents a major shift of how data constraints are defined, impacting a large part of the SilverStripe developer experience.

Options

Option 1: Ad-hoc GraphQL endpoints

Each endpoint represent a particular GridField with a particularDataList. Even though they might return the same data as a generic GraphQL endpoint ("Content API"), they are accessed under a different path.

Code example:

class CommentablePage extends Page {
    public function getCMSFields()
    {
        $fields = parent::getCMSFields();
        $grid = new GridField(
            'ApprovedComments', 
            'All approved comments',
            Comment::get()->filter('Approved', true)
        );      
        $fields->addFieldToTab('Root.Comments', $grid);

        return $fields;
    }
}

Creates a getCommentablePageApprovedComments query, which needs to be known at GraphQL schema compile time (requiring Aaron's GridField Registry).

Option 2: Form-based GraphQL endpoint

Same as above, but uses a generic getGridFieldData() query, reconstructing the Form with its GridField to query its DataList. Does not require all GridFields to be known at GraphQL schema compile time.

Option 3: Generic GraphQL endpoints

There is only one GraphQL endpoint for a particular model. Any modifications to the list of this model (filtering, sorting, paging, custom fields) is expressed through GraphQL resolvers. While developers handle DataList in those custom resolvers, they are writing a generic querying interface with enough capabilities to handle the needs of many GridField implementations.

Code example (not thought out, just to demonstrate the complexities involved here):

SilverStripe\GraphQL\Controller:
  schema:
    scaffolding:
      types:
        Comment:
          fields: [ID, Title, Content]
class CommentablePage extends Page {
    public function getCMSFields()
    {
        $fields = parent::getCMSFields();
        $grid = new GridField(
            'ApprovedComments', 
            'All approved comments',
            Comment::class
        );
        $grid->setQuery(
<<<GRAPHQL
query getComments(approved: true) {
    Created
    Title
    Author
}
GRAPHQL
        );  
        $fields->addFieldToTab('Root.Comments', $grid);

        return $fields;
    }
}

Option 4: GridField providers and registry

See Aaron's RFC

Option 5: DataList auto-converts to GraphQL query

Hard to achieve since we have to cover the whole API surface of DataList, or risk creating a leaky abstraction. For example, you can query relation fields in DataList->filter() through a dot notation (Team::get()->filter('Players.Count():GreaterThan', 10)).

unclecheese commented 6 years ago

RFC is here: https://github.com/silverstripe/silverstripe-admin/issues/556 POC is here: https://github.com/open-sausages/react-gridfield-poc

unclecheese commented 6 years ago

Option 5: DataList auto-converts to GraphQL query

You still have to have an omniscient view of what all these queries are at boot time to put all of them into HOCs.

rupachup commented 6 years ago

the team feels this is done @ingo?

chillu commented 6 years ago

Pulled this back into "In progress", Aaron's looking into an alternative solution - I don't think we're there yet. Probably more than five points by now, but this is important to get over the line properly.

unclecheese commented 6 years ago

Alternative solution

Bearing in mind that we're trying to make the 80% use case as simple and elegant as possible, there is probably a better way to do this.

Summary

Refactor to use a more content API friendly approach. Instead of relying on registries to create magic one-off operations per GridField, we'll rely on the scaffolded operations for each DataObject (all DO's are now universally scaffolded) with the assumption that those operations will be enhanced in future stories (e.g. there is currently no filter argument on READ).

Because the queries are predictable and standardised, we can pass the string representation of the query down the FormSchema response and plug it into the component using the declarative <Query/> component provided by Apollo.

Components are fetched from Injector at render time rather than composed via transforms on boot. This has the biggest risk for a performance impact, as far as I can tell. But doing this through transforms requires action at boot time, which would require a registry, as outlined above.

Advantages

Disadvantages

API Change Proposal

Have chatted about this with @tractorcow, who has vast knowledge of the ORM and his recommendation was to avoid the pitfalls of introspecting a DataList, for many of the reasons stated above. His recommendation was to deprecate passing a DataList instance to GridField and instead rely on an abstraction layer that informs GridField how to build a DataList internally.

$field = ReactGridField::create('MyField', 'Hey this is my field')
   ->setSourceType(Member::class)
   ->setSourceFilters(['Title:PartialMatch', '%damian%']);

This has a similar feel to the old ComplexTableField/ DataObjectManager API (yes, I went there), but it makes more sense given all the layers of abstraction we're going through. Ultimately we want GraphQL, so the idea of going through a DataList just adds more complexity.

Again, keeping in mind we're only going for the 80% use case, this seems pretty palatable. The power users will have access to a setGraphQLQuery method and custom resolvers, etc.

Thoughts?

unclecheese commented 6 years ago

Another open question is how to handle computed fields, which are common in things like summary_fields.

If Member has Name in its $summary_fields declaration, we can query that field. GraphQL fields and DB fields are not one-to-one. But it does need to be explicitly added to the type. The "universal scaffolder" does everything it can, and just adds all the fields in the $db list.

We'll need some way of declaring what getters each dataobject wants to put into its GraphQL type. $summary_fields is a decent proxy for this.

$scaffolder->addFields($allDBFields);
$scaffolder->addFields(array_keys($summary_fields));

But that feels like a lot of coupling.

What if each GridFieldComponent had access to the scaffolder? That way you could have ColumnProvider interfaces make a call to $scaffolder->addFields($summary_fields). Just thinking out loud.

unclecheese commented 5 years ago

A new POC is up using the approach outlined above, where graphql, in literal form, is passed down through the form schema into a <Query> component wrapping a gridfield. This deprecates the need for a registry, which is a good thing.

Thinking about it further, however, this bifurcates the path to query customisation. The new POC builds the query server side, which would ostensibly allow module developers to hook into those query building APIs and influence what gets sent to the react component.

However, we also have a client side API, using injector, that allows query customisation. These two approaches are completely incompatible with each other. The frontend injector is not designed to make updates to a query that is already expressed as a string. This means developers would have to know the difference in implementation -- that GridField is a "server side" GraphQL consumer, and AssetAdmin is fully client side. These types of nuances that put undue strain on developers and raise barriers to entry.

To make the two approaches compatible, I recommend we use abstraction on both sides. GridField will send the form schema only the necessary ingredients to create a query. The creation of the query will then be done client side.

[
    'data' => [
        'graphql' => [
            'name' => 'ReadFiles',
            'params' => [
                // ...
            ]
            'type' => 'QUERY',
            'operation' => 'readFiles',
            'fields' => [
                'pageInfo' => [
                    'totalCount',
                ],
                'edges' => [
                    'node' => [
                        'Filename',
                        'Thumbnail',
                    ]
                ]
            ],
            'fragments' => [
                'FileInterface' // No guarantee this exists. Assumes registered with client?
            ]
        ]
    ]
]

It starts to look an awful lot like an AST, and perhaps for a more standard approach, we could just use pure AST, but well-supported client side libraries that mutate AST aren't easy to find at first glance. Otherwise, an opinionated, declarative structure like this has its merits as well, namely that it would require less configuration and be more adaptable to a set of components that are mostly relying on the same 4-5 types of operations for 90% of use cases.

This raises questions about how we're handling query customisation client side, as well. Right now, while injectable queries are expressed in the abstract, it's a configuration-first API that can get quite cumbersome. Have a look at the POC for injectable asset-admin. There's a lot to consider, and perhaps something more procedural is in order.

const graphql = new QueryBuilder('ReadFiles');
graphql.setType(QUERY)
  .setOperation('readFiles')
  .setParams(...)
  .addFields([
    'Filename',
    'Title',
    new FieldSet('Owners', [
        'FirstName',
        'Surname',
    ]),
  ])

const query = {
  apolloConfig,
  query: graphql
}

This is similar to the PHP query builder that is currently in the React GridField POC

QueryBuilder would invoke some kind of __toString() method just in time to feed the GridField component. Up until then, injector transforms would have their say and have easy access to a common API:

Injector.query.transform('my-transform', updater => {
  updater.query.alterQuery('ReadFiles', graphql => {
    graphql.getFieldList('Root.Owners').addField('Created');
    graphql.getField('Root.Filename').addArg('Relative', 'Boolean!')
  });
});

This is very similar to what we use for form validation.

A less configuration-heavy API would make queries easier to test and debug, but also offer more consistency between module authors and end users.

Further, it would be entirely compatible with a form schema approach:

const graphql = new QueryBuilder().fromJSON(formSchema.data.graphql);