-
Notifications
You must be signed in to change notification settings - Fork 96
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
RFC: React/GraphQL GridField #556
Comments
Discussed with Aaron this morning, he'll try another approach which doesn't come with so much long-term pain (diverging GraphQL APIs between CMS usage and a general-purpose "Content API"). The key point here is figuring out how to avoid Apollo knowing about all GraphQL queries at boot time (through it's HoC). Looks like Apollo v2 gives us more options around this. |
To start out, I'd like to preface this by letting you know ahead of time that I'm not deeply familiar with React (and thus HoC's) nor Apollo and only have a basic understanding of GraphQL. So this will be from the perspective of a heavy and user of fairly advanced functionality the existing Regarding this comment by @chillu on another thread #463 (comment) about the move of GridField to React, I wasn't sure if it was appropriate to comment there, so I'm responding here to close the loop a bit (since it seems more relevant here).
Can you elaborate on what is meant by "DataList vs. GraphQL vs. GridField" here? Or are you simply calling out that the new Also:
But so is Internally, I already have static API's that return pre-configured
So, with that said: Can you please explain precisely why (at least in the current proposal) the API must be architected such that all That leads me to my next question/suggest, I'd like to underscore this point:
We already have two API's at the PHP level, So, to wrap up (and maybe over-simplify a tiny bit) I really like the ideas of:
I just think that we should carefully consider the separation of concerns in config vs. data, maybe config is more static, immutable and etc, but the data underpinning is a strictly separate concern and probably handled with subsequent requests. Also, this implies potentially adding validation to ensure Some complexities: Configuration may need to allow for parameters, such as in the point above about being able to define a parent or category ID. Here's an extremely simple example ( // Setup association ahead of time while editing.
$tabsGridConfig->whenEditing(function(TabListItem $item) {
$item->ContentSectionID = $this->ID;
}); |
Thanks for taking the time to read through all this and provide your feedback, @patricknelson. Lots to respond to, but I'll just hit you with the high level.
|
Overview
This RFC proposes a series of changes to the API and developer experience to support GridFields rendered with React and composed by GraphQL/Apollo data fetching.
Proof of concept
https://github.com/open-sausages/react-gridfield-poc
Key Challenges
getCMSFields()
is assumed non-deterministic, and therefore a non-qualifier for static analysis or evaluation (i.e. through singletons).Proposal
Most of this can be done without major changes to the API, but the changes to the developer experience, particularly around
getCMSFields
will be non-trivial. The fundamental idea behind this proposal is that, while we can't inject at compile time, we can do quite a bit at boot time. This requires that every GridField declares itself on boot and this information is provided to the client to process the necessary transformations inInjector
. Because every GridField needs to declare itself, all GridFields must be uniquely identified.Create a GridField registry
A singleton instance of a GridFieldRegistry is used to store all GridFields in the system by their identifiers. This registry is populated using a method on a common interface, e.g.
provideGridFields(GridFieldRegistry $registry)
, much likeprovidePermissions
.Classes that use GridField become providers
The grid field is then fetched from the registry instead of instantiated in
getCMSFields()
.Ideally, the explicit call to
Injector
could be made more user-friendly with a trait (e.g.getGridField(string $id)
. Or evenGridField::getByIdentifier($id)
Expose all dataobjects to graphql
A simple
ScaffoldingProvider
implementation will suffice.A GridField ScaffoldingProvider runs through the registry and adds the requirements of each GridField
This presumes that GridField lists are
DataList
instances. If they're arbitraryArrayList
/ArrayData
compositions, the developer would need to create his or her own custom GraphQL types and use a custom resolver.Ideally this would be extensible through all the current channels, so that the
SortComponent
could jump in here and add->addSortableColumns(array $fields)
or something like that.LeftAndMain provides client configuration about the registered gridfields.
getFields()
is effectivelyGridField::getColumns()
.getComponents()
would be a serialised representation of the component configuration:A boot function transforms all of the GridField registrations with the
injectGraphql
HOC.This results in the creation of a query
readGridField<MyGridFieldIdentifier>
. Ideally this would use a custom template, other thanread
so it could be disambiguated as a GridField-specific operation in the schema, and we could name it something likeget<GridFieldIdentifier>Data
GridField declares "slots" for component injection
These slots are registered with
Injector
. (Injector.component.register('GridFieldBeforeComponentSlot', GridFieldComponentSlot)
)The assigned components for each GridField are in the config and injected at boot time
GridField components can transform the query via injector.
When the query is injected, the list of components assigned to the GridField is available in the Injector context. This allows components to hook into only those GridField queries that are using the component.
GridField exposes part of the Apollo API to components via context.
Considerations
For this reason, it might make sense to split the GridField API into two sub APIs -- UI and Data. The latter would be frozen after registration. But because UI composition can be conveyed via FormSchema, mutability to UI only (e.g.
->setTitle()
) should be supported as it is now.Much of the above proposal relies on naming conventions, which are quite fragile. We should back these up with selectors and generators to keep things consistent, or add some other layer of abstraction so it doesn't feel so flimsy.
Many of the existing GridField components can keep much of what they have and remain relevant. Things like
DataManipulator
andColumnProvider
really have no concerns about how the data is fetched.Special attention should be given to support the 80% use case, which is CRUD operations for
$this->SomeHasManyRelationship()
. If this gets clunky, we'll have a lot of unhappy developers.Enforcing that
GridField
is not instantiated ingetCMSFields
could be tricky. Heavy-handed solution would be to makeField()
throw. I don't believe there is any case where server-side rendering of a GridField would be appropriate or useful.The text was updated successfully, but these errors were encountered: