Fixed graphql-transport-ws implementation
#528
Conversation
xperiandri
left a comment
xperiandri
left a comment
There was a problem hiding this comment.
Have you found a better spec than this one?
https://github.com/enisdenjo/graphql-ws/blob/master/PROTOCOL.md
It would be nice to follow the naming and add references to the spec as we do in AST code
References to the spec 👍 . Well, this is the official specification. As far as I know, this subprotocol is anyway something more like a de facto protocol than a standard. Or what do you mean by "better spec"? In the Apollo GraphQL documentation, for example, you can see a reference to it: https://www.apollographql.com/docs/react/data/subscriptions#supported-subscription-protocols. |
|
|
||
| type RawServerMessage = { Id : string voption; Type : string; Payload : ServerRawPayload voption } | ||
|
|
||
| [<JsonFSharpConverter(unionTagName = "type", SkippableOptionFields = SkippableOptionFields.Always)>] |
There was a problem hiding this comment.
Granted, this approach uses less code, but the error messages are less helpful, especially for a consumer who is using a whole different platform than dotnet. See the failing tests, for example:
↓ (pos 0)
Expected: "Property "id" is required for this messag"···
Actual: "Missing field for union type FSharp.Data."···
↑ (pos 0)
I think this approach (with JsonFSharpConverter annotations) is fine for application code, i.e. code you'd use in your own internal application, but since FSharp.Data.GraphQL is a library for broader use, I think we should have more customized error messages. So this is the case for us to have our own JSON converter, like in the previous implementation. What do you think?
| open FSharp.Data.GraphQL | ||
| open FSharp.Data.GraphQL.Extensions | ||
|
|
||
| type GQLWebSocketResponse with |
There was a problem hiding this comment.
Do we really need so many overloads right now? One of the things I like about F# is that you can't overload functions, so it feels like a step backwards to use them via classes in F#. I'd prefer to have a record and fill the record accordingly when instantiating it. It's also more visible to reader. I mean: I prefer reading
{ Data = Include data
Path = Skip
HasNext = Skip
Errors = Skip
Extensions = Skip }than reading
GQLWebSocketResponse.Direct(data)because then, as a reader, I can see at a glance what the line is actually about and with GQLWebSocketResponse.Direct(data) it's hidden away from me, so I have to navigate to the code and maybe get lost among the many overload definitions.
There was a problem hiding this comment.
I thought it is too much to fill and transform and it adds unnecessary cognitive load in the place of usage
There was a problem hiding this comment.
I get it, but, as said, I think this is one of those things which look good at a first sight, but have higher costs in the long run. Anyway, this is just my opinion.
| type GQLResponse = | ||
| { DocumentId: int | ||
| Data : Output Skippable | ||
| Errors : GQLProblemDetails list Skippable } | ||
| static member Direct(documentId, data, errors) = | ||
| { DocumentId = documentId | ||
| Data = Include data | ||
| Errors = Skippable.ofList errors } | ||
| static member Stream(documentId) = | ||
| { DocumentId = documentId | ||
| Data = Include null | ||
| Errors = Skip } | ||
| static member RequestError(documentId, errors) = | ||
| { DocumentId = documentId | ||
| Data = Skip | ||
| Errors = Include errors } |
There was a problem hiding this comment.
What do you think about separating concerns for the sake of pull requests? I mean, this pull request was just supposed to fix the graphql-transport-ws implementation, but with this change we're introducing an important breaking change that affects everything else...
I think we should handle those changes in a PR for redesigning the global GQLResponse format.
There was a problem hiding this comment.
OK, let's return your branch to your changes and mine will go next
| yield new RawServerMessageConverter () | ||
| yield! additionalConverters | ||
| } | ||
| let jsonFSharpOptions = jsonFSharpOptions.WithSkippableOptionFields (SkippableOptionFields.Always, true) |
There was a problem hiding this comment.
Can it be the reason why some tests are failing (tests in which the response object now doesn't contain properties that have null values anymore)? I don't know how this configuration here for websockets would leak into these seemingly unrelated tests, though...
Data should be in property "data" and errors should be in property "errors"
"Error" is only intended as a response for "Subscribe" and includes of course any exception thrown during processing of "Subscribe".
It was not being set because in the calls in the overloads it was explicitly being set to `null`.
Because it's path and not a URL :)
xperiandri
force-pushed
the
fix-graphql-transport-ws-impl
branch
2 times, most recently
from
5c9cac1 to
2f0ee57
Compare
xperiandri
changed the title
graphql-transport-ws implementation
2 participants
Here I fix the graphql-transport-ws implementation.
Nextmessages was wrong: it was not the standard{ "data": {}, "errors": [] }(i.e.:ExecutionResult) but just the data object directly.Erroris now only sent as a response to aSubscribemessage and not elsewhere;/wspath not being set (not related to the graphql-transport-ws subprotocol, but anyway to websockets configuration).