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

Not able to execute useLazyQuery multiple times with different variables #9755

Closed
rafeca opened this issue May 23, 2022 · 21 comments · Fixed by #10698
Closed

Not able to execute useLazyQuery multiple times with different variables #9755

rafeca opened this issue May 23, 2022 · 21 comments · Fixed by #10698

Comments

@rafeca
Copy link

rafeca commented May 23, 2022

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:

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).

@dfalling
Copy link

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).

const execute = useCallback<
LazyQueryResultTuple<TData, TVariables>[0]
>(executeOptions => {
execOptionsRef.current = executeOptions ? {
...executeOptions,
fetchPolicy: executeOptions.fetchPolicy || initialFetchPolicy,
} : {
fetchPolicy: initialFetchPolicy,
};

@rafeca
Copy link
Author

rafeca commented Jul 27, 2022

@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).

@sievins
Copy link

sievins commented Jul 27, 2022

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;
}

@konsalex
Copy link

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

@louleo
Copy link

louleo commented Aug 23, 2022

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(...)

@seisenreich
Copy link

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.

@sschneider-ihre-pvs
Copy link

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

@dfalling
Copy link

@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.

@MattLJoslin
Copy link

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?

@sschneider-ihre-pvs
Copy link

@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
Copy link

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.

@RichMatthews
Copy link

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

@jerelmiller
Copy link
Member

jerelmiller commented Dec 19, 2022

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.

@rafeca
Copy link
Author

rafeca commented Dec 19, 2022

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?

@jerelmiller
Copy link
Member

@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!

@Cathy12
Copy link

Cathy12 commented Jan 13, 2023

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!!

@ZakRabe
Copy link

ZakRabe commented Mar 22, 2023

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

@jerelmiller
Copy link
Member

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!

@danReynolds
Copy link
Contributor

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?

@jerelmiller
Copy link
Member

jerelmiller commented Apr 26, 2023

@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!

@github-actions
Copy link
Contributor

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 github-actions bot locked as resolved and limited conversation to collaborators May 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.