-
Notifications
You must be signed in to change notification settings - Fork 61
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
Scaffold object type inheritance #22
Comments
I think that this seems to be a reasonable path to get started on. However, I think that the expectation that we get this immutably perfect on the first try is an unreasonable expectation, and so we should mark all such APIs as |
At the moment this is limited to "read files" and "create folder". The remaining API endpoints will be successively moved over to GraphQL. The GraphQL type creators are likely temporary, to be replaced with a shorter scaffolding-based version: silverstripe/silverstripe-graphql#9 silverstripe/silverstripe-graphql#22 silverstripe/silverstripe-graphql#23 RFC at silverstripe/silverstripe-framework#6245
Most of this has been done in #20, but we don't have a union implementation - what's your take on this, @unclecheese? |
So just to clarify, the question is, how can I get all descendants of SiteTree, and show all applicable fields, e.g.
Is that what you're after? |
How does your Without unions:
With unions:
|
Got it now, sorry, I misread your previous example. I think scaffolding union queries makes perfect sense. So when I add the type I guess the question is if that's really what we want to impose on users of the API. (i.e. the |
You can't have it both ways unfortunately - using union types means the client can't rely on a field being present by default, so you need the
I'm leaning towards Option 1, even if it'll make writing queries a bit harder since you have to think about types that you're accessing attributes on. |
I think a lot of this is use-case dependent. When you look at GraphQL in SS as specifically feeding the CMS then all the ancestry and inheritance information is necessary and hence Option 1 looks to be the path of least resistance. However, if you are looking at using SS as a data repository in a COPE scenario (which is mainly the angle I have been testing to date), then Option 4 is closer to what is required, because the front end is looking for pure, device-agnostic content only. Would it be possible to provide either Option 1, or Option 4. configurably through the scaffolder? |
Yeah, that's the best of both worlds, and a good medium-term plan. But the SilverStripe Content API use case comes first in terms of core priorities - we need to start locking down the API surface for queries provided by core (such as |
Thanks for getting this over the line @unclecheese! There's a bit of a gotcha in your |
If the fields are common to all types, you don't need
^ works
^ Still works.
^ fails
^ works |
Have you tried this in a GraphQL client? http://facebook.github.io/graphql/#sec-Unions indicates that it's not possible, and from memory I've tried that when I was researching this ticket (and confirmed it wasn't):
Sticking with the example from your README, the following fails:
So the dev is safe until the type has been defined (due to a |
Funny, Sam and I were talking about this last week, because he was questioning the forced use of Anyway, looks minor enough to leave alone, but the alternative, of course, would be to enforce unions unconditionally. It's probably not that bad an idea. Otherwise, DataObjects with descendants become a special case, and I'd prefer to obfuscate that detail. I think consistency is more important here than brevity. |
Might be a good idea to have a functional test suite that actually uses a (PHP?) GraphQL client to make a bunch of test queries? |
Related to Scaffold input type inheritance
In line with ORM conventions, we want to expose fields from all subclasses when querying the base class (e.g.
SiteTree
). In contrast to the ORM, GraphQL exposes a type or interface for fields of the returned objects. While we could flatten all fields to the base type, it means a lot of fields which don't make sense without further context:Instead, I propose we create distinct types for each subclass, each of which is a superset of its parent class.
This structure allows us to declare multiple types as a union in queries:
The downside is that queries will always have to declare on which type they're expecting fields:
While a type can implement more than one interface, a field (query response) can only declare a interface response type (or a union of types) - see discussion on facebook/graphql. We could use a single interface, but there's no "union of interfaces" in GraphQL.
Sam suggested types implementing multiple interfaces, but given the lack of "union of interface" support, I don't see much value in this. Also, it we can't have all types implement a
SiteTreeInterface
to get around writing...on SiteTree
for fields on the base type - it's either a single interface, or a union of types (verified through a modified ReadFileQueryCreator).In general, the recommended approach makes it slightly harder to automatically construct GraphQL queries (e.g. for a dynamic React-based GridField, or for a full data exporter). But since most JS UI logic should know which fields it needs in components, I'd see this more of an edge case than a problem in practice. For example, the CMS tree view would always just query the base attributes on the
SiteTree
type since it doesn't need to display anything else.There's some tests in the graphql-js reference implementation for this, and an interesting SO discussion on interfaces vs. unions. And a nice Medium posts about interfaces and unions
The text was updated successfully, but these errors were encountered: