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

Ability to query interfaces that a union implements #44

Closed
sdnts opened this issue Feb 4, 2021 · 7 comments · Fixed by #45
Closed

Ability to query interfaces that a union implements #44

sdnts opened this issue Feb 4, 2021 · 7 comments · Fixed by #45

Comments

@sdnts
Copy link

sdnts commented Feb 4, 2021

We're using genql's typings on an upcoming app and its been working really well! It fits right in, and typed queries and mutations are awesome!

There's one area where type support is limited though, and that has to do with interfaces and unions.
When querying against a resource that returns a union, you should also be able to query interfaces that the union's types implement. Currently, it is only possible to query individual types within the union.

Perhaps an example will make this clearer.

So assume this (part of) GraphQL schema

interface ClientError {
  message: String!
}

type ClientErrorNameAlreadyTaken implements ClientError {
  message: String!
}

type ClientErrorNameInvalid implements ClientError {
  message: String!
}

union GenericError =
    ClientErrorNameAlreadyTaken
  | ClientErrorNameInvalid

type Query {
  error: GenericError
}

Here, this query is currently possible:

query q {
  error {
    ... on ClientErrorNameAlreadyTaken { message }
    ... on ClientErrorNameInvalid { message }
  }
}

as

client.query({
  error: {
    on_ClientErrorNameAlreadyTaken: { message: true }
    on_ClientErrorNameInvalid: { message: true }
  }
})

However, the following is also a valid query in GraphQL that genql disallows:

query q {
  error {
    ... on ClientError { message }
  }
}

It'd be awesome if genql could let me query it this way:

client.query({
  error: {
    on_ClientError: { message: true }
  }
})

If I'm being unclear, please let me know!

I'm more than happy to send a PR your way to support this if you like :)

@remorses
Copy link
Owner

remorses commented Feb 4, 2021

This is not that simple to implement, why don't you use the interface directly instead of the union?

interface ClientError {
  message: String!
}

type ClientErrorNameAlreadyTaken implements ClientError {
  message: String!
}

type ClientErrorNameInvalid implements ClientError {
  message: String!
}

type Query {
  error: ClientError
}

@sdnts
Copy link
Author

sdnts commented Feb 4, 2021

So in our case, we'd like to eventually have some error-specific fields as well (the example was rather simplified). So let's say ClientErrorNameAlreadyTaken could return a suggested name as well.

The goal of my second query then, is to fetch common fields from a all types within the union.

I'll also let @jasonkuhrt chime in if he wants

@jasonkuhrt
Copy link

jasonkuhrt commented Feb 4, 2021

@remorses Because not every operation will return every kind of client error, but we want all client errors to share a base interface so that clients can do a catch-all on the errors of an operation because:

  1. Convenience when going deeper on specific errors is not neeeded

  2. Correctness when the API adds more client error cases to an operation. E.g. evolving the API. In this case the client naturally could not have previously selected for the newly added kind of client error. Hence the client should be using the catch all via the interface to guard against this possibility.

More details on this topic here: https://productionreadygraphql.com/2020-08-01-guide-to-graphql-errors

@jasonkuhrt
Copy link

This is not that simple to implement

Would you be open for a PR from us?

@remorses
Copy link
Owner

remorses commented Feb 5, 2021

I will work on this, I already know what to change, i also need to clean up tests because now it's a mess

@remorses
Copy link
Owner

remorses commented Feb 8, 2021

Released in 2.4.0

@sdnts
Copy link
Author

sdnts commented Feb 9, 2021

@remorses Just tried this out, works exactly as expected! Thanks so much for getting this out so incredibly quickly!

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 a pull request may close this issue.

3 participants