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

Add support for fragments, part 0: design sketch #59

Merged
merged 3 commits into from
Aug 28, 2021

Conversation

benjaminjkraft
Copy link
Collaborator

Summary:

Having implemented support for interfaces, it's time to implement
support for fragments. And it turns out there's actually another design
decision I hadn't really thought about when thinking about interfaces!
In this commit I add a sketch of the design -- two proposed designs
really. This one I think will be easier to change later (via a flag),
but opinions are still welcome.

Issue: #8

Test plan:

read it

Copy link
Contributor

@dnerdy dnerdy left a comment

Choose a reason for hiding this comment

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

Like @csilvers, I also like the flattened approach.

It seems like the flattened approach would also work well for spreads in an abstract scope structs are generated for each concrete type. Callers can then use type switches to handle each of the returned types.

To aid code reuse in Go, it might be fruitful to explore embedded structs for common fields (or generally, embedded structs for abstract types) as opposed to having embedded structs that correspond to fragments.

Given this schema:

type Query { a: I }
type A implements I { r: String, s: String, t: String }
type B implements I { r: String, u: String, v: String }
interface I { r: String }

query MyQuery { x { r ...F, ...G } }  # where x is of type I
fragment F on A { s t }
fragment G on B { u v }

the idea would be to generate the structs:

type MyQueryX interface { isMyQueryX(); GetI() MyQueryXI }
type MyQueryXI struct { R string }
type MyQueryXIA struct { MyQueryXI; S, T string } // implements MyQueryX
type MyQueryXIB struct { MyQueryXI; U, V string } // implements MyQueryX

Then callers could access and pass around common fields without the need to type switch (in "generic" code where that makes sense).

```go
// flattened:
type MyQueryAI interface { isMyQueryAI(); GetB() string }
type MyQueryAIA struct { B, C, D string } // implements MyQueryAI
Copy link
Contributor

Choose a reason for hiding this comment

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

If there's a fragment G on X { e f } where X is type X implements I { b: String, e: String, f: String } would we also generate a struct MyQueryAIX struct { B, E, F string}. If yes, then I like the flattening approach; it's easy to reason about separate structs for each concrete type.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, assuming you spread that G into MyQuery as well, we would!

@benjaminjkraft
Copy link
Collaborator Author

It seems like the flattened approach would also work well for spreads in an abstract scope structs are generated for each concrete type. Callers can then use type switches to handle each of the returned types.

Yeah, I think it depends on why you're using an abstract-in-abstract fragment. If you just happen to have a fragment of useful fields lying around, it's definitely simplest if we just flatten those fields in. But if you have a fragment because you want to pass whatever you get to some shared function that accepts LearnableContent, you want to have some shared representing that. One thing we might be able to do is in the case where we know whether the fragment exists or not, we skip the interface wrapper. I can make some updates to the doc to lay that option out.

It may also be worth expanding on the four sub-cases of abstract-in-abstract, which are, if you are spreading a fragment of type I into a selection of type J, I == J, I implements J, J implements I, and none of the above. (Sadly, the latter may come up more than one might expect, because we don't necessarily define all our interface-interface inheritance. But on the other hand, neither do we have many cases where we even have a type that implements multiple interfaces; all the ones I can find are in content-editing land.)

To aid code reuse in Go, it might be fruitful to explore embedded structs for common fields (or generally, embedded structs for abstract types) as opposed to having embedded structs that correspond to fragments.

Interesting idea -- I can see how if you want to use interfaces as code-sharing this might be of greater value; it could replace (or augment) the getters for each field. Note that what you describe is compatible with either approach to fragments; we could do type MyQueryXIA struct { MyQueryXI; F } too. I think -- let me know if you see any reason this isn't true -- that it should be easy to add such embedding later, if we find that the getter-methods aren't sufficient once we integrate the interfaces changes into webapp. My feeling is if we think the getter methods are sufficient, that's simpler, but we may find places where they aren't. But maybe I'll change my mind once I make the above updates for abstract-in-abstract spreads.

Meanwhile, since I think we're all in agreement that inline fragments should get flattened, I'll go ahead and implement that. It may be that that will be enough for a lot of webapp's queries anyway.

Copy link
Contributor

@dnerdy dnerdy left a comment

Choose a reason for hiding this comment

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

Note that what you describe is compatible with either approach to fragments; we could do type MyQueryXIA struct { MyQueryXI; F } too. I think -- let me know if you see any reason this isn't true -- that it should be easy to add such embedding later, if we find that the getter-methods aren't sufficient once we integrate the interfaces changes into webapp.

Yeah, that sounds right to me.

Meanwhile, since I think we're all in agreement that inline fragments should get flattened, I'll go ahead and implement that. It may be that that will be enough for a lot of webapp's queries anyway.

👍. Thanks for capturing the all this context in the design doc!

@benjaminjkraft benjaminjkraft force-pushed the benkraft.interfaces-4 branch 2 times, most recently from e0eaca5 to b8b66f5 Compare August 25, 2021 19:01
@benjaminjkraft benjaminjkraft changed the base branch from benkraft.interfaces-4 to main August 25, 2021 19:02
Having implemented support for interfaces, it's time to implement
support for fragments.  And it turns out there's actually another design
decision I hadn't really thought about when thinking about interfaces!
In this commit I add a sketch of the design -- two proposed designs
really.  This one I think will be easier to change later (via a flag),
but opinions are still welcome.

Test plan: read it

Reviewers: csilvers, marksandstrom, miguel, adam
- the option of having an embed as well as an interface to represent
  GraphQL interfaces
- simplifications to the embedding approach when embedding abstract
  spreads; namely in our structs we can embed the concrete type of the
  fragment (but still generate the interface for code sharing)
- more explanation about the four sub-cases of abstract-in-abstract
  spreads
@benjaminjkraft benjaminjkraft merged commit e99dced into main Aug 28, 2021
@benjaminjkraft benjaminjkraft deleted the benkraft.fragments-0 branch August 28, 2021 01:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants