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

Play support #234

Closed
wants to merge 10 commits into from
Closed

Play support #234

wants to merge 10 commits into from

Conversation

yoohaemin
Copy link
Collaborator

@yoohaemin yoohaemin commented Feb 23, 2020

close #143

  • Cross built for 2.7 and 2.8
  • Json codecs for caliban datatypes
  • Basic Controller

todo

  • working example
  • websocket (in a followup pr)
  • check sbt config is fine w.r.t cross building play versions

@yoohaemin yoohaemin requested a review from ghostdogpr February 23, 2020 08:55
Copy link
Owner

@ghostdogpr ghostdogpr left a comment

Choose a reason for hiding this comment

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

Added some initial feedback. An example like the http4s/akka ones would be nice. And to update the docs but this can be done at the end.

Also, the WebSocket part can be done in a later PR if you want.

@ghostdogpr
Copy link
Owner

Also, I have never used Play so maybe @paulpdaniels and @fagossa can give more feedback.

runtime
.unsafeRunToFuture(
interpreter
.execute(req.body.query, req.body.operationName, req.body.variables.getOrElse(Map.empty))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Potential follow up areas: supporting the persisted-query and the file upload specs.

li => Json.obj("locations" -> Json.arr(Json.obj("line" -> li.line, "column" -> li.column)))

private def encodeLocationInfoAndMessage(li: Option[LocationInfo], message: String) =
li.fold(Json.obj())(li => Json.obj("locations" -> li)) ++ Json.obj("message" -> message)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can simplify to Json.obj("message" -> message, "locations" -> li)

Copy link
Collaborator Author

@yoohaemin yoohaemin Mar 11, 2020

Choose a reason for hiding this comment

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

Edit: 👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

On second thought, it does change the behavior however trivial it is. I feel better leaving it behave the same with circe, wdyt?

Copy link
Collaborator

Choose a reason for hiding this comment

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

How does the behavior change?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually why does the locationinfo writes also pass it’s key name?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for the catch!
I meant your simplification would add field "locations": null, when LocationInfo is empty, where currently it drops the key altogether.
Current circe impl drops the null values.

@ghostdogpr
Copy link
Owner

This change needs to be applied in this adapter too.

@vpavkin
Copy link
Contributor

vpavkin commented Mar 25, 2020

@yoohaemin Hi!
FYI, play-json support was merged. I didn't touch controllers though, so I guess this PR is still viable, just with a more narrow scope.

@ghostdogpr
Copy link
Owner

Closing this one as most code related to play-json has been merged already. The issue remains open for creating the adapter part.

@ghostdogpr ghostdogpr closed this Apr 19, 2020
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.

Add module for Play support
4 participants