urql-graphql / urql

The highly customizable and versatile GraphQL client with which you add on features like normalized caching as you grow.
https://urql.dev/goto/docs
MIT License
8.57k stars 444 forks source link

RFC: Add APQ support for Contentful (CMS) #3415

Closed soleo closed 8 months ago

soleo commented 9 months ago

Summary

Contentful GraphQL API started Automatic Persisted Query support recently, https://www.contentful.com/developers/changelog/#automatic-persisted-queries-support-in-the-graphql-api. However, their way to triggering re-fetch for missing persisted query is different from Apollo's implementation.

Contenful: PERSISTED_QUERY_NOT_FOUND as error code https://www.contentful.com/developers/docs/references/graphql/#/reference/automatic-persisted-queries

Apollo:

{
  errors: [
    { message: 'PersistedQueryNotFound' }
  ]
}

https://www.apollographql.com/docs/react/api/link/persisted-queries/

Proposed Solution

const isPersistedMiss = (error: CombinedError): boolean =>
  error.graphQLErrors.some(
    x =>
      x.message === 'PersistedQueryNotFound' ||
      (typeof x.extensions === 'object' &&
        x.extensions !== null &&
        'contentful' in x.extensions &&
        (x.extensions as unknown as ContentfulExtensions).contentful.code ===
          'PERSISTED_QUERY_NOT_FOUND')
  );

Requirements

A better way is to propose a standardized way to utilize a set of Error Code that all vendors should be using as part of GraphQL Spec

kitten commented 9 months ago

I'm actually not sure it's appropriate for us to support this. To be honest, what stops any given GraphQL HTTP API to implement any response code or shape they want? Especially in this case, Contentful has decided to namespace the error code under a custom extension key, instead of following the defacto standard.

This is even more odd in my opinion based on their request input shape following the defacto spec.

Overall, most APIs currently follow the defacto spec, and until a spec is ratified and implemented by the GraphQL over HTTP spec extensions, I don't think there's a good argument to be made for implementing arbitrary spec divergences in the urql repo.

There's of course an argument to be made for these functions to be configurable: https://github.com/urql-graphql/urql/blob/0a763bb8229e0bf3f24aa30794f7f62b224750e4/exchanges/persisted/src/persistedExchange.ts#L25-L29

However, again, this hasn't happened because we for now have a defacto spec, and there's no point in diverging from it just sometimes.

So, for now, I can only offer you to please "fork" @urql/exchange-persisted, or rather vendor the exchange into your codebase with the necessary changes

Hope that makes sense to you. Happy to answer more questions about it, but I think for now let's put the PR on hold and I don't think there'll be much movement in urql re. APQ support unless a new spec arises for it in GraphQL over HTTP

soleo commented 9 months ago

Thanks @kitten for your comment. We also submitted support ticket for Contentful to take a look to align with popular GraphQL client out there, e.g Relay, ApolloClient. For now, we will use my own fork.

It would be nice that the error could be part of graphQL spec though.

JoviDeCroock commented 8 months ago

I am going to close this out as it's basically a vendor-specific ask đŸ˜…