urql-graphql / urql

The highly customizable and versatile GraphQL client with which you add on features like normalized caching as you grow.
https://urql.dev/goto/docs
MIT License
8.61k stars 450 forks source link

fix(graphcache): Prevent offlineExchange from issuing duplicate operations #3200

Closed kitten closed 1 year ago

kitten commented 1 year ago

Follow-up to #3199 Resolves #3093

Set of changes

frederikhors commented 1 year ago

I forgot to write here too that this still doesn't fix https://github.com/urql-graphql/urql/issues/3093 as is unfortunately.

See https://github.com/urql-graphql/urql/issues/3093#issuecomment-1528796774.

kitten commented 1 year ago

@frederikhors: Should be fixed now. In short, coming back to this, I think it's best to just force the flushed operations to always use cache-first.

frederikhors commented 1 year ago

Ok. I tried. It works, but.

The cached data is used on the cold load now. ✅

The are no multiple requests for the same call. ✅

But I realized that now on cold start on page reload there is no longer the loading indicator I had before, using the following custom exchange:

const pendingRequestsExchange: Exchange = ({ forward }) => {
  function before(op: Operation) {
    if (op.kind === 'subscription') return;
    if (op.kind && op.kind === 'teardown') {
      console.log('call deletePendingRequest');
      deletePendingRequest(op.key);
    } else {
      console.log('call addPendingRequest');
      addPendingRequest(op.key);
    }
  }
  function after(op: OperationResult) {
    deletePendingRequest(op.operation.key);
  }
  return (ops$) => {
    const forward$ = pipe(ops$, tap(before));
    return pipe(forward(forward$), tap(after));
  };
};

This is happening because it now calls immediately addPendingRequest and deletePendingRequest without waiting the pending request.

This is not what I expect during the cold start with the cache data already there.

frederikhors commented 1 year ago

Added reproduction for the latest issue here: https://github.com/urql-graphql/urql/issues/3093#issuecomment-1569869068.

kitten commented 1 year ago

@frederikhors: I can't debug your exchange for you, but you will have to adjust your after call to be more precise. If this is just to keep track of global loading states, I'd suggest to be more accurate with cache-only operations in after and/or maybe adding a delay or debounce here.

Edit: For now, I'll consider it solves, since I'm assuming your after sees an itermediary result

frederikhors commented 1 year ago

Thank you for the answer, @kitten.

you will have to adjust your after call to be more precise

The thing is the after() function is NEVER called on the first load. LOL.

Is firstly called the before else branch with op:

{"key": 6065549779, "kind": "query", ...}

and then IMMEDIATELY the before if (op.kind && op.kind === 'teardown') branch with op:

{"key": 6065549779, "kind": "teardown", ...}

Can you help me understand if this is a "new behavior" or is my exchange?

I do not understand why the teardown operation is issued. If this is a cache-and-network call it should wait for the server answer.

This only happens at the reload of the page, on what I call "cold start".

This new behavior is reproduced here: https://codesandbox.io/p/sandbox/issue-urql-6-duplicated-requests-forked-0ybcue. You can see the after call is never issued.