Expand mutation and introspection support #193
Conversation
There was a problem hiding this comment.
First: Thanks for putting this together! I'm definitely interested in merging.
I can't add much to your comments. Execution strategy is down to the monad, as you saw we're not explicitly enforcing serial execution for top-level mutations. As long as we're not actively worse off after this PR than before I think it's fine as-is.
Would you mind elaborating a bit what __Type > __Field > __Type means? I'm not super knowledgeable about the introspection spec. "Just Works" definitely a good goal!
tests/EndToEndTests.hs
Outdated
| } | ||
| |] | ||
|
|
||
| -- it "can fetch the __schema" $ do |
There was a problem hiding this comment.
Are the commented out tests aspirational?
There was a problem hiding this comment.
Yeah ... I'll fix / expand / uncomment (or remove) any of them before finalizing this for sure.
|
@teh - Awesome! I'll be working on finalizing and polishing this over the weekend then. On the The spec'd schema would need to look like type Type__ = Object "__Type" '[]
'[ Field "fields" Field__
]
type Field__ = Object "__Field" '[]
'[ Field "type" Type__
]and I got the impression (from an error or comment or something; I don't remember what now) that we couldn't (currently?) support that recursive type definition. I'm hazy on that though; if the above actually does compile, then I should be able to add that part as it stands. |
FWIW, graphql-api was implemented against a version of the spec just before October 2016, which was properly released after we did our initial release of graphql-api. (FWIW, I distinctly remember there not being versions of the spec then, but https://facebook.github.io/graphql/ seems to belie my memory). Anyway, I would love it if we could bring the library up to supporting June 2018 fully. |
|
@jamesdabbs-procore you are right, recursion can't be expressed directly with type UserQ = Object "User" '[] -- this is your GraphQL Object
-- recursion breakers:
newtype RecUserQ = RecUserQ (Handler QueryMonad UserQ)
instance HasResolver QueryMonad RecUserQ where
type Handler QueryMonad RecUserQ = RecUserQ
resolve (RecUserQ handler) = resolve @QueryMonad @UserQ handler
instance HasAnnotatedType RecUserQ where
-- getAnnotatedType returns garbage but it works for now because
-- we're not using it. See https://github.com/jml/graphql-api/issues/93
getAnnotatedType = getAnnotatedType @Text |
| Mutation _ _ _ -> resolve @m @mutations mh (Just ss) | ||
|
|
||
| -- | Interpret a query or mutation against a SchemaRoot | ||
| interpretRequest |
There was a problem hiding this comment.
What's the difference between interpretRequest and executeRequest? Is interpretRequest including the parsing of the document?
|
|
||
| data DefinitionType = QueryDefinition | MutationDefinition deriving (Eq, Show) | ||
|
|
||
| getDefinitionType :: AST.QueryDocument -> Maybe Name -> Maybe DefinitionType |
There was a problem hiding this comment.
Would you mind adding a comment explaining a bit more about this function does? E.g. I'm not super clear on why DefinitionType needs to be wrapped in a Maybe.
| -- AST.TypeNamed $ AST.NamedType $ getName t | ||
| AST.TypeNonNull $ AST.NonNullTypeNamed $ AST.NamedType $ getName t | ||
| typeToAST (TypeList (ListType t)) = | ||
| -- AST.TypeList $ AST.ListType $ AST.TypeNamed $ AST.NamedType $ getName t |
There was a problem hiding this comment.
do you still need the commented out lines? If so maybe add a TODO saying what needs to be done to uncomment them?
|
@jml Not super sure why the coverage ratchet passed for 8.0 but not for 8.2. Do you have any insight / ideas? |
|
We're not running it for 8.0—it's of little extra value for the time it would take to set up. |
|
It's not documented very well, but here's what'll happen with the coverage ratchet:
The reason we fail when coverage improves is to make sure that step 3 actually happens. Otherwise, improvements in one PR could be undone by the next. This way, we lock in any improvements, and move closer to the goal of 100% coverage. |
|
Hey guys. Thanks for this PR. Schema introspection is something I have been looking forward to. Would you like any help here? |
👋 Hello! I've been doing some work to expand this library to support the pi-base project, and - while I don't think this is quite ready to go - I wanted to start a conversation about what if any of this you'd be open to merging upstream. The main things I'm looking for are:
schema.gql- the pi-base is currently writing that to disk here so that the typescript client can read it and make sure the frontend stays in syncqueryandmutationdocument; if we're going to declare those, we might as well actually use them at dispatch time__typenameintrospection so that the client can cache records on(__typename, id).TODO
I currently intend to tackle these on this branch
Introspectionto cover as much of the introspection spec as it reasonably can without major internal changes (the__Type>__Field>__Typepart currently can't be modeled, and I don't plan on touching subscriptions or extensions yet)InterfaceandUnionhandling (I don't use either of these in my project, so just haven't thought about them)Some open questions / thoughts
queriesshould probably be resolved in parallel if possible and top-level fields inmutationsmust (per the spec) be resolved serially. I'm not considering this a blocker for this PR, since the current story for running mutations is "just mutate stuff in a query handler" (I never send a query doc containing more than one mutation anyways), but if this seems important to address here, I can take a look.Let me know if you'd be open to pulling in work in this direction. Notes welcome if there are any tweaks at all that you'd like made here, large or small.