apollo-client icon indicating copy to clipboard operation
apollo-client copied to clipboard

Not able to execute `useLazyQuery` multiple times with different variables

Open rafeca opened this issue 3 years ago • 5 comments

I'm not 100% sure whether this is a valid use case for useLazyQuery(), but I still think this new behaviour is not intended because of the following reasons:

  • The behaviour has changed between v3.5.10 and v3.6.x.
  • The new behaviour does not seem to be right, since the promise that gets resolved after executing a query with some variables corresponds to the response from another query executed with different variables.

Intended outcome:

When executing the same lazy query twice on the same tick with different variables, Apollo should execute two network requests each one with the corresponding variables and return the correct results

const [fetch] = useLazyQuery(CountriesList);

useEffect(() => {
  Promise.all([
    fetch({variables: {continent: "EU"}}),
    fetch({variables: {continent: "NA"}})
  ]).then(([result1, result2]) => {
    // result1 should contain the results of executing CountriesList query with continent=EU
    // result2 should contain the results of executing CountriesList query with continent=NA
  });
}, [fetch]);

Actual outcome:

When executing the same lazy query twice on the same tick with different variables, Apollo instead executes a single network request and returns the same results for the two requests.

So, on the code snipped pasted above, both result1 and result2 contains the results of executing CountriesList query with continent=NA.

How to reproduce the issue:

  • CodeSandbox with [email protected] (issue reproducible): https://codesandbox.io/s/winter-sea-1i2wpn?file=/src/App.js
  • CodeSandbox with [email protected] (issue not appearing): https://codesandbox.io/s/silly-resonance-c8nj8i

The expected behaviour is the two fetch calls on L27 and L28 to return different results, and therefore the UI should show 2 columns with different sets of countries.

Versions

@apollo/client >= 3.6.0 (the issue is also present in the currently latest available alpha (v3.7.0-alpha.7).

rafeca avatar May 23 '22 20:05 rafeca

I'm seeing this issue as well. useLazyQuery writes the request data to a ref, so inside useQuery.asyncUpdate all the calls have the same data (the last request).

https://github.com/apollographql/apollo-client/blob/a7970cf3e06a44b695350d6dd7326a3f087294c8/src/react/hooks/useLazyQuery.ts#L74-L82

dfalling avatar May 29 '22 20:05 dfalling

@benjamn have you had any chance to look at this issue? It would be good to hear your thoughts on this (specially whether this is considered a bug or intended behaviour for us to plan ahead).

rafeca avatar Jul 27 '22 13:07 rafeca

I have just created an overly complicated 😬 workaround for this ... you probably don't want to use this but thought I'd share.

Basically it's a hook which staggers the lazy queries when invoked multiple times in the same tick, and resolves them sequentially.

Note: It uses ramda and uuid

Usage

const Component = () => {
  const [fetch] = useLazyQuery(QUERY);
  const staggeredFetch = useStagger(fetch);

  useEffect(() => {
    Promise.all([
      staggeredFetch({variables: {continent: "EU"}}),
      staggeredFetch({variables: {continent: "NA"}})
    ]).then(([result1, result2]) => {
      // result1 and result2 are different
    });
  }, [staggeredFetch]);

  return <></>
}

Overly complicated hook implementation

import React, { useCallback, useEffect, useMemo, useState } from "react";
import { v4 as uuid } from "uuid";
import any from "ramda/src/any";
import assoc from "ramda/src/assoc";
import curry from "ramda/src/curry";
import find from "ramda/src/find";
import isEmpty from "ramda/src/isEmpty";
import map from "ramda/src/map";
import propEq from "ramda/src/propEq";
import reduce from "ramda/src/reduce";
import when from "ramda/src/when";
import without from "ramda/src/without";

type QueuedItem = {
  args: any[];
  id: string;
  result?: any;
  status: "inProgress" | "resolved" | "pending";
};

class Target extends EventTarget {}

/**
 * useStagger is a higher order hook that returns a function, which when
 * invoked multiple times resolves each invocation sequentially; one at a time.
 * This effectively staggers a function when invoked multiple times.
 *
 * @param executable
 */
export default function useStagger<Args, Resolve>(executable: Function) {
  const [queue, setQueue] = useState<QueuedItem[]>([]);
  const target = useMemo(() => new Target(), []);

  useEffect(() => {
    const isItemToProcess = !isEmpty(queue);
    const isItemInProgress = any(propEq("status", "inProgress"), queue);

    if (!isItemToProcess || isItemInProgress) return;

    const resolvedItem = find(propEq("status", "resolved"), queue);

    if (resolvedItem) {
      const { result } = resolvedItem;
      target.dispatchEvent(new CustomEvent(resolvedItem.id, { detail: { result } }));
      setQueue((_queue) => without([resolvedItem], _queue));
    } else {
      const itemToProcess = queue[0];
      setQueue((_queue) => alter({ key: "status", value: "inProgress" }, itemToProcess.id, _queue));
      (async () => {
        const result = await executable(...itemToProcess.args);
        setQueue((_queue) =>
          alterAll(
            [
              { key: "status", value: "resolved" },
              { key: "result", value: result },
            ],
            itemToProcess.id,
            _queue
          )
        );
      })();
    }
  }, [queue]);

  return useCallback(
    (...args: Args[]) => {
      const deferred = new Deferred<Resolve>();

      const id = uuid();
      setQueue((_queue) => [..._queue, { id, args, status: "pending" }]);

      target.addEventListener(id, (e) => {
        if (!isCustomEvent(e)) {
          throw new Error("not a custom event");
        }
        deferred.resolve(e.detail.result);
      });

      return deferred.promise;
    },
    [executable, queue]
  );
}

/**
 * alter a specific object's properties within an array
 */
const alter = curry((
  { key, value }: { key: string, value: any },
  id: string,
  items: any[]
) => map(
  when(propEq('id', id), assoc(key, value)),
  items
))

/**
 * alter multiple properties of a specific object within an array
 */
const alterAll = (
  keyValuePairs: { key: string; value: any }[],
  id: string,
  items: any[]
) => (
  reduce((acc, { key, value }) => (
    alter({ key, value }, id, acc)
  ), items, keyValuePairs)
);

/**
 * class which enables promises to be resolved/rejected outside their internal scope
 */
class Deferred<Resolve> {
  promise: Promise<Resolve>;
  reject!: (reason?: any) => void;
  resolve!: (value: PromiseLike<Resolve> | Resolve) => void;
  constructor() {
    this.promise = new Promise((resolve, reject) => {
      this.reject = reject;
      this.resolve = resolve;
    });
  }
}

/**
 * type guard to narrow Event type to a CustomEvent
 *
 * @param event
 */
function isCustomEvent(event: Event): event is CustomEvent {
  return "detail" in event;
}

sievins avatar Jul 27 '22 20:07 sievins

Faced the same issue to with version "@apollo/client@^3.6.9" had to downgrade to 3.5.9

konsalex avatar Aug 11 '22 13:08 konsalex

Got this issue as well. Another work around would be useApolloClient directly.

  const client = useApolloClient();
  const promises = Array.map((item) => {
    return client.query({
     query: YOUR_QUERY_DOCUMENT,
     variables: {
     YOUR VARIABLES  
   }  
  })
  });
 Promise.all(promises).then(...)

louleo avatar Aug 23 '22 23:08 louleo

We have the same problem, downgraded to 3.5.10 did work but is pretty disappointing.

How does it come that this bug hasn't been fixed since may? We're at 3.7.0 now. Executing a lazy query seems to be a pretty much basic and widely used functionality for me. Just curious.

seisenreich avatar Oct 13 '22 14:10 seisenreich

not sure but https://www.apollographql.com/docs/react/api/react/hooks/#refetch isn't actually refetch the thing that should be used?

sschneider-ihre-pvs avatar Oct 18 '22 08:10 sschneider-ihre-pvs

@sschneider-ihre-pvs, no- refetch is for updating a single query...for new data from the server, or to pass in new args. The problem with useLazyQuery is many of us would like to use it to fire off multiple simultaneous requests, caring about the results of each.

In my case I'm requesting a URL from my server that I can use to upload a file. If I have two files I want to upload in parallel, and send two unique requests to my server for URLs, Apollo replies to both requests with just one of the responses.

dfalling avatar Oct 18 '22 10:10 dfalling

Hey I've also encountered this bug, trying to understand what the ETA is on this? This seems to be a pretty serious regression from the previous behavior and should probably have come with some kind of alternative or at least a release note to indicate this basic behavior was changing. Anything I can do to help resolve this?

MattLJoslin avatar Dec 13 '22 18:12 MattLJoslin

@sschneider-ihre-pvs, no- refetch is for updating a single query...for new data from the server, or to pass in new args. The problem with useLazyQuery is many of us would like to use it to fire off multiple simultaneous requests, caring about the results of each.

In my case I'm requesting a URL from my server that I can use to upload a file. If I have two files I want to upload in parallel, and send two unique requests to my server for URLs, Apollo replies to both requests with just one of the responses.

Oh okay so all in parallel sure then refetch does not work I guess.

sschneider-ihre-pvs avatar Dec 14 '22 09:12 sschneider-ihre-pvs

what about squeezing all queries into one payload? Would that be an option? Like

query lazy1 {
  querySomething(input: 'a') {
    id
    name
  }
}

query lazy2 {
  querySomething(input: 'b') {
    id
    name
  }
}

query lazy3 {
  querySomething(input: 'c') {
    id
    name
  }
}

Queries are executed in parallel on the server while mutations are in sequence.

sschneider-ihre-pvs avatar Dec 14 '22 09:12 sschneider-ihre-pvs

experiencing this issue too and having to use client.query as a workaround

RichMatthews avatar Dec 19 '22 13:12 RichMatthews

Hey all 👋! Thanks so much for your patience with this issue. I'd like to best understand the expected behavior here and see if we can come to a resolution. I can see both sides of the argument in this case where this could either be viewed as a bug or intended behavior.

Something to note is that useLazyQuery returns a data and loading property in the 2nd item of the tuple. This allows you to render you component with that data, without you needing to manage that state yourself. From our docs:

import React from 'react';
import { useLazyQuery } from '@apollo/client';

function DelayedQuery() {
  const [getDog, { loading, error, data }] = useLazyQuery(GET_DOG_PHOTO);

  if (loading) return <p>Loading ...</p>;
  if (error) return `Error! ${error}`;

  return (
    <div>
      {data?.dog && <img src={data.dog.displayImage} />}
      <button onClick={() => getDog({ variables: { breed: 'bulldog' } })}>
        Click me!
      </button>
    </div>
  );
}

Note here how we use data to render the component. In this case, you can just execute the query without needing to await the promise or manage the state yourself.


I can definitely see the merit in wanting to execute the query in the same tick with multiple arguments. My question is: what would you expect the values of data, loading, and error to be when you executed this bit of code?

const [fetch, { data, loading }] = useLazyQuery(CountriesList);

useEffect(() => {
  Promise.all([
    fetch({variables: {continent: "EU"}}),
    fetch({variables: {continent: "NA"}})
  ]).then(([result1, result2]) => {
    // result1 should contain the results of executing CountriesList query with continent=EU
    // result2 should contain the results of executing CountriesList query with continent=NA
  });
}, [fetch]);

We have to be careful here since this would result in a race condition for these properties.

I generally view useLazyQuery as a way to delay execution of a single query (query in this case meaning a single chunk of data from your server including its lifecycle) rather than a means to arbitrarily query data. In this vein, I think useLazyQuery is working as expected.

For your particular example, you might be better off using client.query anyways directly as it doesn't come with the lifecycle baggage of useLazyQuery, nor will it unnecessarily rerender your component as you're managing the lifecycle yourself anyways:

import { useApolloClient } from '@apollo/client';

const [countries, setCountries] = useState();
const client = useApolloClient();

useEffect(() => {
  Promise.all([
    client.query({query: CountriesList, variables: {continent: "EU"}}),
    client.query({query: CountriesList, variables: {continent: "NA"}})
  ]).then(([result1, result2]) => {
    setCountries({eu: result1.data.countries, na: result2.data.countries});
  });
}, [client]);

And if you want something to feel more React-y, you can always create a custom hook to wrap this particular behavior:

// warning: not tested
export function useArbitraryQuery(query, options) {
  const client = useApolloClient();

  return useCallback((partialOptions) => {
    return client.query({ ...options, ...partialOptions, query })
  }, [query, options, client])
}

Given your specific example, I'm curious if you'd even need useLazyQuery as I see its executing in useEffect to fetch immediately (though I take this with a grain of salt given that you might be using this to demonstrate the issue). Instead, would it make more sense to split this up between 2 useQuery or useLazyQuery hooks? I think you get the same benefit but the data/loading/error properties are much more predictable:

const [fetchEU, euResult] = useLazyQuery(CountriesList);
const [fetchNA, naResult] = useLazyQuery(CountriesList);

useEffect(() => {
  fetch({variables: {continent: "EU"}})
  fetch({variables: {continent: "NA"}})
}, [fetchEU, fetchNa]);

In this case, this gives you the benefit of just using the result on the tuple instead of needing to manage state yourself.


All this being said, I'd love to understand how you view the expected behavior here. It's possible there is a good case for making a single useLazyQuery hook work with multiple calls to its fetch function in the same tick. I'd love to hear more of your thoughts. If this is the case, I think we need to flush out the expected behavior for the hook lifecycle to ensure it remains working as you'd expect.

jerelmiller avatar Dec 19 '22 19:12 jerelmiller

Hey @jerelmiller! thanks a lot for taking the time into writing such a detailed response.

I filled the issue because this is a change of behaviour in Apollo v3.6.0 which in my opinion is buggy now (resolving the returned promise with a response from a different request is wrong). I agree with you that there are better ways to achieve the example code that I pasted there (and in fact I'm working in limiting the way people use useLazyQuery in my company, the problem is that I'm not going to be able to change all existing code that we already have).

So this issue was initially filled to know whether the Apollo team thinks this change of behaviour is indeed an issue or on the other hand it's intended. We wanted to decide what to do based on what you folks tell us, since this change is preventing us from upgrading to Apollo v3.6 (we have a huge codebase with hundreds of useLazyQuery usages relying on the previous correct behaviour that we cannot easily change at this moment). In the last months we've decided to start working on our own versions of useLazyQuery and useMutation to have more control over their behaviour and rely less on the potential changes of undocumented behaviour.

what would you expect the values of data, loading, and error to be when you executed this bit of code?

In that scenario, I expect data, loading and error to contain the values of the last started execution (no matter whether it started in the same tick of other executions).

For your particular example, you might be better off using client.query anyways directly as it doesn't come with the lifecycle baggage of useLazyQuery, nor will it unnecessarily rerender your component as you're managing the lifecycle yourself anyways

100% agree, but as I mention we have a lot of existing code already using useLazyQuery and depending on the previous behaviour.

And if you want something to feel more React-y, you can always create a custom hook to wrap this particular behavior:

Yes, this is what we're planning to do to be able to upgrade to Apollo v3.6 without having to change all product code. I'm going to create a custom version of useLazyQuery with the previous behaviour to unblock the upgrade. Once we're upgraded my plan is to ask product teams to change their code to not rely on the promise results to eventually be able to limit the API surface of our custom hooks.

Given your specific example, I'm curious if you'd even need useLazyQuery as I see its executing in useEffect to fetch immediately

This was just a minimal repro silly example 😄 I can come back with more realistic examples where lazy queries are executed multiple times if you want (I work in an infra team so I'm not 100% familiar with all the patterns that the product code uses). I know that this pattern is widely used by our list components to do optimistic fetching on pagination.


Related to all this, but also a bit offtopic to this issue, I think there are a few flaws in the APIs of the useLazyQuery (and useMutation) hooks:

  1. The callback function returning a Promise with data, error and loading: first, having both loading and the Promise is unnecessary, we just need one loading indicator. Same with error and promise rejection (the promise currently gets rejected only when there are network errors but this is undocumented behaviour and it has also changed in the past and may change in the future as well).
  2. Also, the fact that the returned data exists in both the callback and the hook creates some duplication that cause confusion (should I trust the data returned from the hook or the one returned from the callback?).
  3. Both the callback and the hook accept variables. By debugging, it's clear that there's shallow variable merging happening, but its behaviour is not documented and it falls apart in some specific use cases, for example when the passed variables change on both sides (e.g a variable that's passed to the hook and overriden by the callback changes between renders).

As I mentioned before, to solve these flaws our end goal is to have our own custom capped down versions of useLazyQuery()/useMutation() that only returns data on the hook (the callback promise resolves to void) and only accepts variables on the callback.

Are these flaws something that the Apollo team sees as a problem?

rafeca avatar Dec 19 '22 21:12 rafeca

@rafeca thanks so much for this detail. This is extremely helpful understanding the context surrounding this issue.

I'll try and respond to bits of this over time, but I'd love to take this back to the Apollo team and throw some ideas/questions around. FWIW, I agree that there are a few quirky things with useLazyQuery (like the loading flag in the resolved promise you pointed out). We'll discuss and figure out if we deem this buggy or not.

Thanks again!

jerelmiller avatar Dec 19 '22 21:12 jerelmiller

Got this issue as well. Another work around would be useApolloClient directly.

  const client = useApolloClient();
  const promises = Array.map((item) => {
    return client.query({
     query: YOUR_QUERY_DOCUMENT,
     variables: {
     YOUR VARIABLES  
   }  
  })
  });
 Promise.all(promises).then(...)

Got the same issue. This works for me. Thanks @louleo, you save my day!!

Cathy12 avatar Jan 13 '23 05:01 Cathy12

Ran into this issue in 2023. ended up making something similar to the client.query example above by @jerelmiller

ZakRabe avatar Mar 22 '23 19:03 ZakRabe

Hey @rafeca 👋

Thanks so much for your patience on this issue! We just released v3.7.11 which now corrects the behavior. If you kick off multiple requests in parallel, each promise will now resolve with the correct data. The hook will always reflect the last execution of the query. Thanks for reporting the issue!

jerelmiller avatar Mar 31 '23 19:03 jerelmiller

Awesome stuff @jerelmiller ! It looks like only the last execution is cached though. Is there a way to get all the parallel requests in the cache?

danReynolds avatar Apr 25 '23 17:04 danReynolds

@danReynolds would you mind opening a separate issue for that? I'm not sure if this is expected or buggy, but I'd love to track that in a separate issue. Thanks!

jerelmiller avatar Apr 26 '23 16:04 jerelmiller

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. For general questions, we recommend using StackOverflow or our discord server.

github-actions[bot] avatar May 27 '23 00:05 github-actions[bot]