Create a Request object and an interpretRequest path #128
Conversation
teh
left a comment
teh
left a comment
There was a problem hiding this comment.
Really cool - thanks for your work!
I'm a bit confused by the mempty behaviour in this context because I haven't looked at the code in a while.
src/GraphQL/Internal/Syntax/AST.hs
Outdated
| instance Aeson.FromJSON Name where | ||
| parseJSON = Aeson.withText "Name" $ \v -> | ||
| case makeName v of | ||
| Left _ -> mempty |
There was a problem hiding this comment.
Is mempty the right thing here? Should we maybe fail instead? I'm not even sure why mempty is a valid value for Name - maybe @jml remembers :)
There was a problem hiding this comment.
In both this and the other case, you're running inside the Aeson Parser monad. Protolude is a little more aggressive about moving fail out of the base Monad class, so it looks like I need to import Control.Monad.Fail and then it works.
There was a problem hiding this comment.
@teh This is a wart in Aeson IMO. Because mempty wasn't prefixed with return (or pure), it's mempty :: Aeson.Parser Name, not mempty :: Name (which wouldn't compile).
If you look at the source for Parser a, it has an implementation of Monoid such that mempty = fail "mempty". I think this kind of sucks.
empty (i.e. failed Alternative, not monoid identity), would be much more correct.
But fail is best, because it's informative.
src/GraphQL/Value.hs
Outdated
| parseJSON (Aeson.Number x) = return $ ConstFloat $ toRealFloat x | ||
| parseJSON (Aeson.Bool x) = return $ ConstBoolean x | ||
| parseJSON Aeson.Null = return ConstNull | ||
| parseJSON _ = mempty |
There was a problem hiding this comment.
What's the mempty for ConstScalar?
There was a problem hiding this comment.
It's not mempty for ConstScalar, it's mempty for Parser a.
|
Hi @dmaze, Thanks for this PR—it seems like a good idea. I'd like to have a look over it before merging to make sure I understand it. Out of curiosity, can you share a little about what you're using graphql-api and servant for? Thanks, |
|
My day job is using GraphQL pretty successfully, and in particular, the React+Relay+Flow JavaScript stack has been a pretty comfortable way to write front-end applications that don't need much state beyond what they can get from the server. For my personal tasks I prefer to use languages where, say, it's possible for my editor to notice simple typos. The specific thing I'm actually playing with is an application that reads the MBTA's (Boston public transit agency) real-time data feeds, caches them in a Persistent (SQLite) database, then re-serves the cached data to a front-end that can display the actual travel times between a pair of stops on a day-to-day basis (while the T claims the Red Line has a 92% on-time rate, it feels to my like at least 25% of my commute-time trips have significant delays). If the back-end exposed a GraphQL interface, then the front-end would be very familiar space to me...which brought me here. |
jml
left a comment
jml
left a comment
There was a problem hiding this comment.
Thanks! Sorry for the delay in reviewing & the multiple round-trips—we're still figuring this out.
|
|
||
| instance FromJSON scalar => FromJSON (Object' scalar) where | ||
| parseJSON = Aeson.withObject "Object" $ \v -> do | ||
| -- Order of keys is lost before we get here |
There was a problem hiding this comment.
Can you remind me why this is relevant?
| -- Note that the 'FromJSON' instance always decodes JSON strings to | ||
| -- GraphQL strings (never enums) and JSON numbers to GraphQL floats | ||
| -- (never ints); doing a better job of resolving this requires query | ||
| -- context. |
There was a problem hiding this comment.
I'm a little uncomfortable about this. The "Zen of Python" advises "in the face of ambiguity, refuse the temptation to guess", and I think that's good advice.
Some options I could think of:
- don't have a
FromJSONinstance for this, and instead have a series of functions that take the query (or whatever the minimal necessary context is) and aValueto be parsed, so that we can unambiguously parse these things - create a new type (types?) that unites the query context with these values and write
FromJSONinstances for these new types - create a new
AmbiguousConstScalartype. Then, have a function that transformsAmbiguousConstScalar -> ConstScalarby also taking a query context. This type would need fewer branches thanConstScalar. - make
ConstScalara phantom type, where the phantom parameter is whether it's ambiguous or not. Then, have a function that transformsConstScalar Ambiguous -> ConstScalar Unambiguousby also taking a query context
3 participants
There is a mostly-standard JSON request format that's pretty widely support across GraphQL implementations, even if not part of the spec per se. This PR adds a
Requesttype that mirrors that format, and a top-levelinterpretRequestentry point to run it.The real goal of this is to allow a Servant path like
The one big caveat in this PR is that you can't 100% deserialize
Valueobjects without knowing their type context. This assumes that JSON strings are always GraphQL strings (they could also be enums or schema-specific scalar types) and that JSON numbers are always GraphQL floats (they could also be integers and that's probably the common case).