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

Improve UX for constructing a Caliban interpreter via interop #2130

Merged
merged 3 commits into from
Feb 23, 2024

Conversation

kyri-petrou
Copy link
Collaborator

Main changes:

  1. Make GraphQL#interpreterEither public
  2. Deprecate interpreterAsync methods in cats / monix interop in favour of interpreterF / interpreterMonix which don't have an implicit requirement on ToEffect / Runtime

Some notes:

  • We could also add a GraphQL#interpreterUnsafe method which impurely throws the Left if we wish so to make testing simpler
  • Regarding (2): I went with deprecating the methods to maintain binary compatibility. We could instead just change the implicit requirements on the interpreterAsync methods which won't need any user-facing changes but that means that the next version will need to be 2.6.0. Frankly, I prefer just doing this.

I also made changes to the documentation but will add them once we decide what to do with interpreterAsync

@kyri-petrou kyri-petrou merged commit 7dd7d78 into series/2.x Feb 23, 2024
10 checks passed
@kyri-petrou kyri-petrou deleted the graphql-interpreter-either branch February 23, 2024 03:47
@satorg
Copy link
Contributor

satorg commented Mar 8, 2024

@kyri-petrou , regarding

We could instead just change the implicit requirements on the interpreterAsync

Just my couple of cents: I personally like your decision to change the name to interpreterF, because I find the old interpreterAsync rather misleading. Because it is not somehow more "async" comparing to the native interpreter method that returns ZIO. They are both "async" in a similar way. Therefore, interpreterF makes more sense to differentiate it from the native one.

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.

None yet

3 participants