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

RFC: Development mode validation of Graphcache resolver configuration against schema #1293

Closed
Kingdutch opened this issue Jan 12, 2021 · 2 comments · Fixed by #1304
Closed
Labels
needs more info ✋ A question or report that needs more info to be addressable

Comments

@Kingdutch
Copy link
Contributor

Summary

I've just spent two days trying all sorts of things to debug why my pagination wasn't working. The good news is, I now have an intimate understanding of the Urql relayPagination inner workings and its test

(and I see some robustness changes in the tests I'd be happy to make separately)

Cursor nullability

The test seem to indicate that there's a relationship between hasNextPage being false and endCursor being null as well as hasPreviousPage being false and startCursor being null. However, this is incorrect. The Relay cursor specification states

startCursor and endCursor must be the cursors corresponding to the first and last nodes in edges, respectively.

I believe the tests should be updated to reflect the above condition. The cursors in pageInfo should only ever be null when there are no edges. (The spec currently says cursors should be a non-nullable type, contradicting even that, but that's being worked on)

Subset test confusion

The test returns a subset of the cached items if the query requests less items than the cached ones does not do what it states on the tin. Its implementation actually tests that the entire cache is returned when a smaller subset is requested.

The naming of this test is somewhat confusing. The name of the test may do what you expect but the behaviour of the test matches that of the documentation (and behaviour desirable for pagination).

I hope we can use the data that urql has available for itself to save another developer that same time.

Proposed Solution

Would it be possible to validate the resolvers configuration of the Graphcache schema against the schema that is loaded for schema awareness when Urql is used in development mode?

Requirements

When schema awareness is set-up the Graphcache should check the keys for the resolvers entry of its configuration and

  1. Check that all defined types exist, provide a warning if that's not the case
  2. Check that all used types are concrete types. Graphcache currently doesn't apply a resolver for an interface to all its implementing types, so the user should get a warning saying something like Graphcache does not support resolvers for interface types. Please move the resolver for 'Interface' to the concrete types 'SomeImplementsInterface', 'OtherImplementsInterface'.
@Kingdutch Kingdutch added the future 🔮 An enhancement or feature proposal that will be addressed after the next release label Jan 12, 2021
@kitten
Copy link
Member

kitten commented Jan 12, 2021

So, all of these validations do exist: https://github.com/FormidableLabs/urql/blob/0b2c8db694dd5541d47c033108d8bd975e699e38/exchanges/graphcache/src/store/store.ts#L94-L102

Could it be that you'd maybe want to submit a bug ticket if they're slightly inaccurate at times?

@kitten kitten added needs more info ✋ A question or report that needs more info to be addressable and removed future 🔮 An enhancement or feature proposal that will be addressed after the next release labels Jan 12, 2021
@Kingdutch
Copy link
Contributor Author

Kingdutch commented Jan 12, 2021

Ah I thought they were missing for my scenarios since I hadn't seen any on the console.

It's indeed true that "1. Check that all defined types exist, provide a warning if that's not the case" is covered by https://github.com/FormidableLabs/urql/blob/0b2c8db694dd5541d47c033108d8bd975e699e38/exchanges/graphcache/src/ast/schemaPredicates.ts#L202

However, the scenario I ran into is that I defined a resolver for an interface. However, resolvers for interface types are never actually used since the __typename returned by API calls, that is used by the cache, are always a concrete type.

I'd like to propose replacing https://github.com/FormidableLabs/urql/blob/0b2c8db694dd5541d47c033108d8bd975e699e38/exchanges/graphcache/src/ast/schemaPredicates.ts#L204 with the following

} else if (schema.types[key].kind === "INTERFACE" || schema.types[key].kind === "UNION") {
  warnAboutAbstractResolver(key);
} else {

Where warnAboutAbstractResolver is implemented as

function warnAboutAbstractResolver(name: string, kind: 'UNION' | 'INTERFACE'): void {
  warn(
    `Invalid resolver type: \`${name}\` does not match to a concrete type in the schema, but the \`resolvers\` option is referencing it. Implement the resolver for the types that ${kind === 'UNION' ? 'make up the union' : 'implement the interface'} instead.`,
    26
  );
}

This would help catch the scenario that I ran into sooner and provides a message on how to resolve this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs more info ✋ A question or report that needs more info to be addressable
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants