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

Conditionally skip REST calls #299

Open
ryanrhee opened this issue Aug 8, 2022 · 8 comments
Open

Conditionally skip REST calls #299

ryanrhee opened this issue Aug 8, 2022 · 8 comments
Labels
feature Feature: new addition or enhancement to existing solutions

Comments

@ryanrhee
Copy link

ryanrhee commented Aug 8, 2022

(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
Copy link
Author

ryanrhee commented Aug 8, 2022

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

@fbartho fbartho added the feature Feature: new addition or enhancement to existing solutions label Aug 8, 2022
@fbartho
Copy link
Collaborator

fbartho commented Aug 8, 2022

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!

@ryanrhee
Copy link
Author

ryanrhee commented Aug 8, 2022

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

@fbartho
Copy link
Collaborator

fbartho commented Aug 8, 2022

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.

@ryanrhee
Copy link
Author

ryanrhee commented Aug 8, 2022

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
Copy link
Author

ryanrhee commented Aug 9, 2022

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.

@fbartho
Copy link
Collaborator

fbartho commented Aug 9, 2022

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.

@ryanrhee
Copy link
Author

ryanrhee commented Aug 9, 2022

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Feature: new addition or enhancement to existing solutions
Projects
None yet
Development

No branches or pull requests

2 participants