skip option fails when assigned a variable that flips from false to true

[sirugh]

Intended outcome:

skip: true results in no network request.
The error property in the result of the useQuery hook reflects the current running query.

Actual outcome:
skip: true results in a network request.
The error property displays for the previous result.

How to reproduce the issue:

1. Go here: https://codesandbox.io/s/apolloclient-v3-skip-bug-ov1z3?file=/src/app.js
2. Click the button and observe the 400 from the graphql endpoint.
3. Click the button again and observe the successful response but the logged error message.

Versions

Related Issues
#6190
#6572
#6507

[sirugh]

I traced all the way into the fetchQueryByPolicy function introduced in #6221. I believe it has something to do with the skip option not being passed to the resultsFromLink call:

apollo-client/src/core/QueryManager.ts

Lines 999 to 1005 in 2de0e23

	const resultsFromLink = (allowCacheWrite: boolean) =>
	this.getResultsFromLink<TData, TVars>(queryInfo, allowCacheWrite, {
	variables,
	context,
	fetchPolicy,
	errorPolicy,
	});

I tried locally modifying this but even when passing skip the network request is still made.

[sirugh]

@benjamn I'd like to learn more about the inner-workings of ApolloClient, and would be happy to try and fix this but I'm not sure what I'm looking for. I know the options are differing here, with skip: true being in the new options:

apollo-client/src/react/data/QueryData.ts

Lines 241 to 255 in 7993a77

	if (
	!equal(
	newObservableQueryOptions,
	this.previousData.observableQueryOptions
	)
	) {
	this.previousData.observableQueryOptions = newObservableQueryOptions;
	this.currentObservable
	.setOptions(newObservableQueryOptions)
	// The error will be passed to the child container, so we don't
	// need to log it here. We could conceivably log something if
	// an option was set. OTOH we don't log errors w/ the original
	// query. See https://github.com/apollostack/react-apollo/issues/404
	.catch(() => {});
	}

which calls to update the "current observable" on the query data class. I'm guessing that for some reason the thing that makes fetch/no-fetch decisions is not aware of this update.

[Ericnr]

I'm having this same problem.

[vojto]

Is this a duplicate of apollographql/react-apollo#3492 (comment)? Also related: #6342

[sirugh]

Is this a duplicate of apollographql/react-apollo#3492 (comment)? Also related: #6342

Maybe? I wanted to create an issue with very simple/specific repro.

[vojto]

Okay so skip has been not working for a while. This comment fixed it for me, until a new version is released:

It actually broke in 3.0.0-beta.46. Last working release is 3.0.0-beta.45!

[cdaz5]

i'm surprised the major release (out of beta) was done with this still broken (since it was a known thing not a bug that popped after release).. it's a pretty widely used feature and I was a little shocked after I migrated to find the console error and this thread .. would of been nice if it was a known thing to make mention of it in the release/migration guide .. maybe its just me though?

[3nvi]

Funnily enough we migrated back to beta-43. It's more stable as of the moment of writing

[cdaz5]

@3nvi is there any other things I should keep an eye on based on your comment sounds like there might be other unmentioned issues? .. we've been QA'ing the migration to 3.0.2 but a heads up if you know of anything specific would be great!

[3nvi]

@3nvi is there any other things I should keep an eye on based on your comment sounds like there might be other unmentioned issues? .. we've been QA'ing the migration to 3.0.2 but a heads up if you know of anything specific would be great!

#6603
#6644
#6636 (fixed in upcoming 3.1.0.)

[tafelito]

related to #6572

[s-taylor]

If anyone needs a work around, this is what I came up with.

import { OperationVariables } from '@apollo/react-common';
import { DocumentNode } from 'graphql';

import { useEffect } from 'react';
import { useLazyQuery, LazyQueryHookOptions } from '@apollo/react-hooks';

const useSkippableQuery = <TData = any, TVariables = OperationVariables>(
  query: DocumentNode,
  options: LazyQueryHookOptions<TData, TVariables> & { skip: boolean }
) => {
  const { skip, ...rest } = options;

  const [lazyQuery, result] = useLazyQuery<TData, TVariables>(query, rest);

  useEffect(() => {
    if (!skip) lazyQuery();
  }, [skip]);

  /* On the first render cycle, lazy query always returns loading false because the useEffect has
   * not yet invoked the query. This ensures that the loading will be true on the first cycle
   * by checking the skip prop directly and checking the query has been called */
  const loading = result.loading || (!skip && !result.called);

  const data = !skip ? result.data : undefined;

  return { ...result, data, loading };
};

export default useSkippableQuery;

You can then use this pretty much like useQuery and provide the skip prop. It's not heavily tested but I think gets the job done.

[sirugh]

@s-taylor nice hook! I was going down the route of switching us to lazy query with effects/skip flags but I got hung up on the initial loading state. I'll give this hook a shot :) Thanks for the suggestion!

Edit: @s-taylor, oops, forgot that data is switched to undefined when skip toggles from false to true. You'll want to add something like:

    const data = !skip ? result.data : undefined;

    return { ...result, data, loading };

Edit 2: We need to pass variables to the invoker since the variable can change between renders, but for some reason, passing the variables to the effect cause an infinite loop. :/

    const { skip, variables, ...rest } = options;
    const [runQuery, result] = useLazyQuery(query, rest);

    useEffect(() => {
        if (!skip) {
            runQuery({
                variables
            });
        }
    }, [runQuery, skip, variables]);

[tafelito]

as a workaround, you might want to try this #6572 (comment)

You don't need to use useEffect nor useLazyQuery and avoid having 2 render cycles

TLDR

const skip = !company;
  const variables = !skip
    ? {
        companyId: company.id,
      }
    : {};
  const { data, loading } = useQuery({
    variables,
    skip,
  });

[sirugh]

@tafelito does not work for us - query is still invoked when skip toggles from false to true.

[s-taylor]

@sirugh nice thanks! I will update the snippet.

One flaw I've found with my approach though, is that once skip has been set to false and the lazy query has been triggered, it seems like the lazy query is always re-fired on a re-render. I guess this is the lazy query behaviour, but this is a problem for us as we sometimes want the query to go back to being skipped.

[hwillson]

We're finalizing a fix for this and other skip related issues, and should have it ready today.

[hwillson]

@sirugh would you be able to verify if @apollo/[email protected] fixes your original issue?

[sirugh]

@hwillson I'll give it a check after my morning meetings. Can you give me about two hours? If you publish pre's to npm you could also check the sandbox quickly, I believe.

Edit: Looks like you guys do, and looks like it did fix the issue in the sandbox. As discussed on the PR, data is being returned now when skip: true, but at least no network request is made.