awslabs / aws-mobile-appsync-sdk-js

JavaScript library files for Offline, Sync, Sigv4. includes support for React Native
Apache License 2.0
919 stars 266 forks source link

AWSAppSyncClient does not respect disableOffline option in a custom ApolloLink using createAppSyncLink #359

Open pckilgore opened 5 years ago

pckilgore commented 5 years ago

Do you want to request a feature or report a bug? Bug

What is the current behavior? When using a custom apollo-link, aws-appsync will never respect the users' disableOffline: true setting, and thus will never use the users' custom cache (and possibly other features behind a disableOffline flag).

For us, the clue this was happening was inexplicable fragment matching errors...

You are using the simple (heuristic) fragment matcher, but your queries contain union or interface types. Apollo Client will not be able to accurately map fragments. To make this error go away, use the IntrospectionFragmentMatcher as described in the docs

...despite us setting up the appropriate fragment matcher in our custom InMemoryCache.

The issue in the code originates here, where the disableOffline is defined only based on the config passed in as the first argument:

https://github.com/awslabs/aws-mobile-appsync-sdk-js/blob/2278942104ca96aead029075461ae554dc4a0408/packages/aws-appsync/src/client.ts#L171-L183

which then skips the custom cache here:

https://github.com/awslabs/aws-mobile-appsync-sdk-js/blob/2278942104ca96aead029075461ae554dc4a0408/packages/aws-appsync/src/client.ts#L202-L204

If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem.

To reproduce:

const awsAppSyncLink = createAppSyncLink({ 
  disableOffline: true,
  /* other settings */
})
const cache = new InMemoryCache({ fragmentMatcher: () => { /* stuff */} })
const link = ApolloLink.from([/* other links */, awsAppSyncLink ])
const client = new AWSAppSyncClient({ }, {link, cache})

What is the expected behavior? My custom cache will be used.

Which versions and which environment (browser, react-native, nodejs) / OS are affected by this issue? Did this work in previous versions?

aws-appsync@^1.0.0:
  version "1.7.1"
  resolved "https://registry.npmjs.org/aws-appsync/-/aws-appsync-1.7.1.tgz#bb2c2afbc9c36b736932a1159472fee9450b501d"
  integrity sha512-9mURKvwxtJpyAcpkCAeyq4FEbaXCdu4BwgQxEL4tYK7vvFyz5f5BmUdvla1qOAdqP2swRQ+zaWow1ZxDGrFuDw==

Current workaround

Pass the disableOffline option into the first argument (same code above, fixed):

const awsAppSyncLink = createAppSyncLink({  /* settings */ })
const cache = new InMemoryCache({ fragmentMatcher: () => { /* stuff */} })
const link = ApolloLink.from([ /* other links */, awsAppSyncLink ])
const client = new AWSAppSyncClient({ disableOffline: true }, { link, cache })
pckilgore commented 5 years ago

This is solved either with better documentation, or refactoring the AWSAppSyncClient to look more like this:

export interface AWSAppSyncClientOptions {
    url: string,
    region: string,
    auth: AuthOptions,
    conflictResolver?: ConflictResolver,
    complexObjectsCredentials?: CredentialsGetter,
    cacheOptions?: ApolloReducerConfig,
    disableOffline?: boolean,
    offlineConfig?: OfflineConfig,
    // new below
    customClient?: Partial<ApolloClientOptions<TCacheShape>>
}

so that custom configurations will always carry the correct options.

jpaas commented 5 years ago

Agree with the sentiments above. This appears to have all started with PR #96 which although made it possible to add custom Apollo links, was a bit non-obvious in terms of how it was done, and never added any docs. So people have relied on anecdotal evidence from Github issues on how to add custom links.

jschertz commented 4 years ago

Ran into a related issue attempting to override AWSAppSyncClient's built-in retry logic using Apollo's apollo-link-retry Link. Our goal was to reduce the number of total retries in the event of a network error.

Initially, we were disabling offline within the createAppSyncLink options. In this case, RetryLink was ignored completely.

...

const retryLink = new RetryLink({
  delay: {
    initial: 300,
    max: Infinity,
    jitter: true,
  },
  attempts: {
    max: 2,
    retryIf: (error, _operation) => !!error,
  },
});

const cache = new InMemoryCache();
const stateLink = withClientState({ cache, resolvers, defaults });
const appSyncLink = createAppSyncLink({
  url: graphqlUrl,
  region: appsyncRegion,
  auth: authConfig,
  disableOffline: true,
});

const link = ApolloLink.from([stateLink, retryLink, appSyncLink]);
const client = new AWSAppSyncClient({}, { link });

...

Moving disableOffline:true outside of the custom link allows RetryLink overrides to function

const client = new AWSAppSyncClient({ disableOffline: true }, { link });

Is this intended functionality? Is our use of Apollo's RetryLink at odds with AWSAppSyncClient's expectations? Also, It would be ideal to document this in greater detail and provide more robust code examples.

jasonb-medopad commented 4 years ago

Hi all, this thread has been super helpful, thanks!

I've been trying in vain to implement the same functionality as @jschertz. I'm also trying to add some custom response handling so that something else happens if I get a 302 response from my /graphql endpoint (which can happen for reasons that can't change, in this context).

I'm using aws-appsync v1.8.1 which, from what I can gather, includes the changes from the infamous #96.

A stripped back version of my client config is this:

  const errorLink = onError(({ graphQLErrors, networkError }) => {
    debugger;
    if (graphQLErrors)
      graphQLErrors.map(({ message, locations, path }) =>
        console.log(
          `[GraphQL error]: Message: ${message}, Location: ${locations}, Path: ${path}`,
        ),
      );

    if (networkError) console.log(`[Network error]: ${networkError}`);
  });

  const appSyncLink = createAppSyncLink({
    url: GRAPHQL_API_URL,
    region: AWS_REGION,
    auth: {
      type: AUTH_TYPE.AMAZON_COGNITO_USER_POOLS,
      jwtToken: token
    }
  });

  const retryLink = new RetryLink({
    attempts: {
      max: 1,
      retryIf: errorResponse => {
        const errors = errorResponse.result.errors;
        let shouldRetry = false;
        // some other stuff goes here
        return shouldRetry;
      }
    },
    delay: (count, operation, error) => {
      console.log('delaying...');
      return 2 * 1000;
    },

  });

  const link = ApolloLink.from([
    errorLink,
    retryLink,
    appSyncLink
  ]);

  return new AWSAppSyncClient({ disableOffline: true }, { link });

Question 1:

In the onError links, should I be able to see the console.log() / have the debugger triggered on every network error/graphql error? Currently, I see a network error sometimes (if I shut down my backend dev server is, for example) - but not all the time.


Question 2:

If there is a network error, should I be able to see the console.log in the delay parameter of my RetryLink? At the moment, I can't


Miscellaneous questions: