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

lazyQuery - double network call #9448

Closed
nudelx opened this issue Feb 23, 2022 · 36 comments · Fixed by #11403
Closed

lazyQuery - double network call #9448

nudelx opened this issue Feb 23, 2022 · 36 comments · Fixed by #11403

Comments

@nudelx
Copy link

nudelx commented Feb 23, 2022

Hi
I have updated my @apollo/client from 3.50 to the latest release 3.5.8
and I have an issue with the lazyQuery, which creates double network calls:

Example

 const [getUserInfo, { data, error, called, networkStatus }] =  useLazyQuery(GET_USER_INFO)
 
  useEffect(() => {
    if (total === 42) {
      console.log('call for user info')
      getUserInfo()
    }
  }, [total, getUserInfo])
 

When running this useEffect hook, I see the console log message issued only once. However, the server executes the
query twice in the DB and I see two network calls in the browser inspect

OPTIONS OPTIONS FETCH FETCH
and following these, 2 queries in the DB.
I tried to use fetchingPolicies but without any success
when I downgrade to 3.5.0 then it works as expected

@metamorfoso
Copy link

I've encountered the same issue.

3.5.7 is the latest version without this behaviour.

@yuriibut
Copy link

yuriibut commented Mar 8, 2022

same for me

@deizianens
Copy link

deizianens commented Mar 9, 2022

Same here, also using useLazyQuery on @apollo-client version 3.5.9

@Akallabet
Copy link

I'm using version 3.3.14 and have the same issue...

@benjamn
Copy link
Member

benjamn commented Apr 5, 2022

@nudelx (and others) Could you try the latest v3.6 beta by running npm i @apollo/client@beta? I believe PR #9564 will improve this behavior.

@benjamn benjamn added this to the Release 3.6 milestone Apr 5, 2022
@benjamn
Copy link
Member

benjamn commented Apr 26, 2022

We've merged release-3.6 into main and released PRs #9564 and #9599 in @apollo/[email protected] just now. That version is still tagged as next rather than latest on npm, but we will promote it to latest after some final testing. Thanks for opening this issue and for your patience waiting for the solution to land!

@ChechurinSerhiy
Copy link

ChechurinSerhiy commented Apr 27, 2022

@apollo/client": ^3.6.0
The second request really disappeared, but for some reason, the loading status does not change to false after the request has already been completed

@Anakhom
Copy link

Anakhom commented Nov 25, 2022

encountering the same issue still on 3.6.9

@mrocznyKucykZaglady
Copy link

mrocznyKucykZaglady commented Apr 27, 2023

3.7.10
I also encountered this issue and made some investigation. In my case changing variables causing an issue:
First run:

variables: {
    search: "something",
    flag: "true"
}

Second run:

variables: {
   flag: "true"
}

In this scenario I see two request to DB. First one is cancelled ( with search ), second is ok (without search).

Adding condition in useLazyQUery for variables solves the problem, and i see always only one call:

export function useLazyQuery(query, options) {
var _a;
    var execOptionsRef = useRef();
    var optionsRef = useRef();
    var queryRef = useRef();
    var merged = execOptionsRef.current ? mergeOptions(options, execOptionsRef.current) : options;
    /**SWEET**/
    if (options.variables) {
        merged.variables = options.variables
    }
    var document = (_a = merged === null || merged === void 0 ? void 0 : merged.query) !== null && _a !== void 0 ? _a : query;
    optionsRef.current = merged;
    queryRef.current = document;

It looks like merge options in second call merge old options ( with search ) then call again without.
For sure merge is needed here (fetchPolicy etc.) but for variables, when new ones are being send they should not be merged with older ones.

@Kicks456
Copy link

Kicks456 commented Jul 27, 2023

In any sort of authentication flow this is a big deal, where we 429 block IP's making too many requests, or 423 block users after "n" amount of failed attempts.

Still having this issue with version: 3.7.17

Below is example snippet of calling a useLazyQuery function

const [login, { error, loading }] = useLazyQuery(LOGIN, {
      fetchPolicy: 'no-cache',
  });
    
 const onSubmit = async (values) => {
      const formIsValid = isValid(values);

      if (formIsValid) {
          return await login({
              variables: {
                  credentials: {
                      username: values.username,
                      password: values.password,
                  },
              },
          });
      }
  };

Proof from Chrome Network tab that the call is being made twice:

image

@Kicks456
Copy link

Disregard ^^^, went digging in client, and found the way I had set up my link chain, forwarding an operation in an invalid location was the issue.

@jerelmiller
Copy link
Member

Hey all 👋

We'd greatly appreciate a reproduction of this issue to dig in further. We had a number of fixes for useLazyQuery go out with v3.7.4 and v3.7.11 that may or may not address the issue originally seen here.

You can use our error template as a starting point or send us a Replay of the issue which would help tremendously. Thanks!

@jerelmiller jerelmiller added the 🏓 awaiting-contributor-response requires input from a contributor label Jul 27, 2023
@sa-ma
Copy link

sa-ma commented Aug 4, 2023

It seems to be fixed on v3.7.4 but it occurs on v3.7.11. I'll find some time this weekend to create a replay of the issue

@github-actions github-actions bot removed the 🏓 awaiting-contributor-response requires input from a contributor label Aug 5, 2023
@asvetlenko
Copy link

I have updated version from "@apollo/client": "3.7.10", to "@apollo/client": "3.8.4", and get twice requests for one call getData

const [getData, { loading: dataLoading }] = useLazyQuery<GetCandidates, GetCandidatesVariables>(
    GQL,
    {
      fetchPolicy: 'no-cache',
      notifyOnNetworkStatusChange: true,
    },
  )

@phryneas
Copy link
Member

@asvetlenko We cannot reproduce this and really need to see this with our own eyes. Please create a reproduction or a Replay.

You can use our error template as a starting point or send us a Replay of the issue which would help tremendously. Thanks!

@bignimbus
Copy link
Contributor

@sa-ma thanks for the Replay! Since that link is public I'll delete your original comment out of caution and copypaste just the text below:

It seems to be fixed on v3.7.4 but it occurs on v3.7.11. I'll find some time this weekend to create a replay of the issue

The issue is back and I have no idea why. I updated the library to v3.8.4 and still encountered the error.

@apollographql apollographql deleted a comment from sa-ma Oct 12, 2023
@phryneas
Copy link
Member

@sa-ma, just to be on the safe side, could you please set the Replay to "private" and share it with [email protected] directly?

@sa-ma
Copy link

sa-ma commented Oct 13, 2023

@sa-ma, just to be on the safe side, could you please set the Replay to "private" and share it with [email protected] directly?

I will. Thanks.

@sa-ma
Copy link

sa-ma commented Oct 25, 2023

Hi,

Is there any update on this or has anyone found a workaround?

@phryneas
Copy link
Member

phryneas commented Nov 6, 2023

@sa-ma in your case, I could find the error, and it seems to be on your side: Your Layout component creates a new Apollo Client instance on every re-render (<ApolloProvider client={client(logOutAndReload)}>), and as a consequence the new Apollo Client has an empty cache and needs to make another request. That Layout rerender seems to happen directly after you get your request.

@palyvodaBoi
Copy link

palyvodaBoi commented Nov 27, 2023

In my case, useLazyQuery called twice when I delete one of the existing query variables:

const [ fetchSomething ] = useLazyQuery(gql(QUERY))

fetchSomething({ variables: { filter: some_filter, orderBy: some_sorting }}) // calls once
fetchSomething({ variables: { orderBy: some_sorting }}) // calls twice: with old filter value and without it

But how to fix it IDK, coz GQL do not allow empty object variables ( filter: {} )

@phryneas
Copy link
Member

@palyvodaBoi could you please create a reproduction for this? This has been extremely hard for us to reproduce, and we really need to see this happening.

@palyvodaBoi
Copy link

palyvodaBoi commented Nov 27, 2023

@phryneas, thank you for your quick response. It's the same tough for me to create a Jsfiddle with an example. Do you have a playground?

@phryneas
Copy link
Member

@palyvodaBoi
Copy link

palyvodaBoi commented Nov 30, 2023

@phryneas it was working, but now:
The web page at https://codesandbox.io/p/devbox/github/apollographql/react-apollo-error-template/tree/main/ might be temporarily down or it may have moved permanently to a new web address

Maybe i can show my component and Apollo setup here?

@phryneas
Copy link
Member

@palyvodaBoi We really need to see this running - at this point, I've tried to reproduce this at least five times.
I've checked the link, and the CodeSandbox seems to be working again. Alternatively, you can clone https://github.com/apollographql/react-apollo-error-template and add the reproduction there.

@palyvodaBoi
Copy link

@phryneas thanx a lot! Finally, it works for me. Here is the branch with my setup and changes (only ApolloClient.tsx and index.jsx were changed): https://github.com/palyvodaBoi/react-apollo-error-template/tree/bug/useLazyQuery-double-call

There are 2 buttons fetching data with useLazyQuery
Push the first button (with GQL variables) => one API call
Then
Push the second button (with no GQL variables) => two API calls (one with old GQL variables, another one with new/empty)

First button:
Screenshot 2023-12-01 at 13 32 49

Second button:
Screenshot 2023-12-01 at 13 33 22
Screenshot 2023-12-01 at 13 33 34

@jerelmiller
Copy link
Member

jerelmiller commented Dec 1, 2023

@palyvodaBoi thanks so much for that reproduction! This is extremely helpful.

From what I've observed, it looks like this becomes an issue when you start with variables, then call it again without any variables. If you start by clicking the 2nd button (as many times as you want), you'll see a single network request issued. Clicking the 1st button also issues a single request. Its only after you click the 2nd button again after you've set variables that the issue shows up.

This is definitely a huge clue as to what might be happening here. Thank you so much for the help! We will see if we can get a fix in here soon 🙂

Edit: I've got a failing test that reproduces this in #11403

@jerelmiller
Copy link
Member

jerelmiller commented Dec 1, 2023

@palyvodaBoi if you need a workaround, you can fix the double request by setting that variable value to undefined.

const [fetchFilms] = useLazyQuery(query)

// first click
fetchFilms({ variables: { filmId: 'ZmlsbXM6MQ==' })

// second click
fetchFilms({ variables: { filmId: undefined } }) // undefined removes the filmId key in the request

This bug in particular gets a bit tricky due to the mechanics of useLazyQuery and how we merge options between the stuff passed to the hook directly, and the stuff passed to the execute function. In other words, its difficult to determine intent.

Take this example:

const [getFilms] = useLazyQuery(GET_FILMS, { variables: { id: '1' })

// Should fetch the film with id = 1. No variables passed
getFilms()
// Should fetch the film with id = 2 because we are changing variables
getFilms({ variables: { id: '2' } })

The question then becomes, what happens if I call getFilms again with no args?

getFilms()

Should this fetch with id: '2' because it was the last set variable? Should it fetch without an id because I didn't pass anything? Should it fetch with id: '1' because thats whats passed to the hook?

I'd argue the right behavior here is that it should try again with id: '2' because that is the variable that was last called with.

That brings up this current bug. If this behavior becomes the rule (merge variables and use the last set value), this means you won't be able to "clear" a value just by calling the function again.

fetchFilms({ variables: { id: '1' })
fetchFilms() // should try again with id '1'

To me, fixing this bug would mean executing the query with the last set variables again. To "clear" that variable value and avoid it getting sent to the server, you'd need to pass undefined as the value:

fetchFilms({ variables: { id: undefined })

This would ensure that we maintain a consistent behavior no matter how or where those variables were set. Calling the execute function without variables essentially just becomes a "don't override what I set last time".


Does this behavior make sense to you if we end up fixing this bug in that manner?

@palyvodaBoi
Copy link

palyvodaBoi commented Dec 2, 2023

@jerelmiller, i appreciate your support and quick response! Thanx again :)

in my opinion, it is more logical that getFilms() in your example should call a function with initial variables: id: '1'
but... I don't know, it's only from the perspective of my logic :) I'm gonna use id: undefined workaround. It's fine for me!

@mrocznyKucykZaglady
Copy link

Thing about "undefined" is that you need to keep your variables and mapping (put undefined) them everytime they are changed :/ if you put some variable and don't need it more you need to set it to undefined.

What about something like:
If variables are not defined call query with last variables.
If variables are set, overwrite stored.

@jerelmiller
Copy link
Member

@mrocznyKucykZaglady its an interesting idea, but unfortunately I think that would be too much of a breaking change to consider in 3.x. We've allowed the ability to merge variables between the hook and execute function, so completely overwriting the variables would definitely break some usages that expect the merging of those variables.

On the flip side, even if we decided to move forward with this, this would mean you'd need to keep an entire mapping of your variables somewhere, which may end up being more cumbersome than just setting the variable you'd like to remove to undefined.

I think the fix here is just to make sure that calling the execute function with no arguments would send the last used variables and avoid the double call.

@jerelmiller
Copy link
Member

jerelmiller commented Feb 1, 2024

Hey all 👋

I believe I've solved this with #11403. We will try and get this pushed out in the next patch version or so.

If you'd like to try out the snapshot release, you can install it with the following:

npm i @apollo/[email protected]

Looking forward to closing the loop on this one!

@jerelmiller jerelmiller self-assigned this Feb 2, 2024
@jerelmiller
Copy link
Member

After a big more digging at historical behavior on how variables merging works, @palyvodaBoi's opinion was the right one:

in my opinion, it is more logical that getFilms() in your example should call a function with initial variables: id: '1'

My original comment goes to show how our documentation was a bit ambiguous on this topic and how I've always thought about this hook in the wrong way.

@phryneas and I took a dive into early versions of 3.x to verify how the original intended behavior. In versions 3.0.0 and onward until this bug was introduced, variables merging was done by taking the variables passed to the hook and merging it with the variables passed to the execute function. This means that if you do not pass variables to the execute function, the only variables sent with the request are the ones passed to the hook itself.

#11403 fixes the double network request and ensures the variable merging behavior works as originally intended. I've added a note in our docs to try and clarify this point to avoid ambiguity going forward.

Copy link
Contributor

github-actions bot commented Feb 7, 2024

Do you have any feedback for the maintainers? Please tell us by taking a one-minute survey. Your responses will help us understand Apollo Client usage and allow us to serve you better.

Copy link
Contributor

github-actions bot commented Mar 9, 2024

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 Mar 9, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.