apollo-link-rest icon indicating copy to clipboard operation
apollo-link-rest copied to clipboard

Conditionally skip REST calls

Open ryanrhee opened this issue 2 years ago • 8 comments

(accidentally hit enter, sorry!)

I have a nested @rest() field (e.g. author) in my graphql query that utilizes an @export()ed field (e.g. authorID) to make a query. Sometimes, the exported field is not set. In those cases, I don't want to make the REST call at all. (In this example, if the authorID is not present, calling /get_author doesn't make sense.)

A way to skip the query conditionally would fix this.

Same as #259.

ryanrhee avatar Aug 08 '22 15:08 ryanrhee

(side note: tried to add the feature label by checking the box, but the label didn't get applied.)

ryanrhee avatar Aug 08 '22 15:08 ryanrhee

I’d be happy to review a design proposal for how this would work. My cautious guess is that it will be pretty difficult to implement something clean for this functionality. — You’re effectively trying to squeeze in a feature that doesn’t exist in GraphQL, and you’re doing it to control functionality of an extension to GraphQL @rest(…)

Patches on patches = pretty messy. If you can find a clean way to do this let us know!

fbartho avatar Aug 08 '22 16:08 fbartho

How do you feel about adding a skip: parameter to the @rest() directive?

ryanrhee avatar Aug 08 '22 16:08 ryanrhee

I worry that we have to return with an object that matches the provided type. If skip is true how will those interact?

If skip is true, then the type you pass to @rest(…) now needs to be an optional type. I can see really weird error messages popping out in places in that situation.

Maybe I’m over-cautious though.

fbartho avatar Aug 08 '22 16:08 fbartho

I think you're talking about the type: parameter on the @rest() directive, right?

I would expect the type parameter to be just match the schema type, which would be optional.

Here's an example:

schema {
  query: Query
}
type Query {
  getAllAuthors: [Author!]!
}
type Author {
  # Imagine that some authors do not have ID set.
  id: ID
  name: String!
  # This is the field fetched via REST link.
  #
  # Schema defines this as optional,
  # since authors without IDs can't fetch their books.
  bookLink: BookLink
}
type Book {
  id: ID!
  authorID: ID!
  title: String!
}
type BookLink {
  books: [Book!]!
}
query GetAuthors {
  getAllAuthors {
    id @export(as: "authorID")
    name
    bookLink
      @rest(
        type: "BookLink"  # Type matches the schema type
        path: "/booksByAuthor/{exportVariables.authorID}"
        skip: 
      ) {
      books {
        title
      }
    }
  }
}

Defining this type as non-optional would just mean that the schema is wrong. Even if the skip: parameter wasn't defined, you could imagine the field being optional because the underlying REST call fails, and a custom error handler provides a fallback empty response.

So I think the type: parameter should match the schema type, regardless of the skip: parameter. That makes it easy to reason about. What do you think?

ryanrhee avatar Aug 08 '22 18:08 ryanrhee

Another thing to point out is that the apollo client useQuery has a skip that also makes for a less ergonomic type.

Extending the above example, if we had a by-ID author fetch:

query GetAuthor($id: ID!) {
  getAuthor(input: {id: $id}) {
    name
  }
}

And we used this in a query where the ID might be null or undefined:

const router = useRouter()
const id: string | undefined = router.query["id"]
const { data } = useQuery(GetAuthorDocument, {
  variables: {
    id
  },
  skip: !id,
})

This fails to typecheck, because the expected TS type of id here is string, coming from the graphql type ID!. We'd have to change the variables to something like this:

const idOrEmpty: string | undefined = router.query["id"]
const id = idOrEmpty ?? "" // default to string of length 0 to satisfy type constraint
const { data } = useQuery(GetAuthorDocument, {
  variables: {
    id
  },
  skip: !id,
})

for it to type check correctly.

So I think there's precedent for a slight loss of ergonomics when using a skip: parameter in the apollo client itself, and so I think it's OK for a similar loss of ergonomics to exist in the REST link.

ryanrhee avatar Aug 09 '22 16:08 ryanrhee

I see where you’re coming from, and if this were just in TypeScript I’d tend to agree.

This optional arg actually has to apply across two type systems which I think makes it more complicated.

You’re welcome to submit a non-breaking PR for this feature. And I’d be happy to take a look.

fbartho avatar Aug 09 '22 16:08 fbartho

thanks for your consideration. I'll submit a PR.

ryanrhee avatar Aug 09 '22 16:08 ryanrhee