Cache exchange not forwarding teardown correctly

[federicobadini]

Hi,

While I was working on a custom exchange I think I have found something wrong with how the default cache exchange forward requests down to the other exchanges downstream.
I have created a codesandbox to show you the problem.

Basically I have created an exchange that deduplicates queries by cancelling the old request and forwarding the new one (switchMapExchange).
In the example attached, I issue a couple of query at the same point in time.
You'll see that even though I forward teardown actions to cancel the first request of the two, no request is effectively cancelled and unexpectedly both are completed.

Trying to solve the problem, I'm quite convinced that it derives from how the default cacheExchange handle the forward action.
It basically creates different forwarding stream for new and skipped ops and merge those streams afterwards

const newOps$ = pipe(
  sharedOps$,
  filter(op => !shouldSkip(op) && !isOperationCached(op)),
  map(mapTypeNames),
  forward,
  tap(response => {
    if (response.operation.operationName === 'mutation') {
      handleAfterMutation(response);
    } else if (response.operation.operationName === 'query') {
      handleAfterQuery(response);
    }
  })
);

const skippedOps$ = pipe(
  sharedOps$,
  filter(op => shouldSkip(op)),
  forward
);

return merge([cachedOps$, newOps$, skippedOps$]);

In the codesandbox linked above I have included also a slightly modified cache exchange, in which forwarded operation streams are handled differently

const forwardedOps$ = pipe(
  merge([
    pipe(
      sharedOps$,
      filter(op => !shouldSkip(op) && !isOperationCached(op)),
      map(mapTypeNames)
    ),
    pipe(
      sharedOps$,
      filter(op => shouldSkip(op))
    )
  ]),
  forward,
  tap(response => {
    if (response.operation.operationName === "mutation") {
      handleAfterMutation(response);
    } else if (response.operation.operationName === "query") {
      handleAfterQuery(response);
    }
  })
);

return merge([cachedOps$, forwardedOps$]);

This approach solved my problem forwarding correctly teardown actions to default fetchExchange and cancelling request when needed.
Please let me know what you think

[kitten]

That seems absolutely right 👍 forward should also not be called twice. It's possible, but obviously leads to subtle bugs, since it spreads out some of the code.

I can put a fix in as soon as I find the time 🙌 Thank you so much for reporting this! I appreciate that this must have been hard to hunt down!

We should likely implement a test suite that can be run on all exchanges individually to test their compliance. Maybe even add a warning if forward is called multiple times as this will often not be intentional 🤔

For the record, I'm working on a normalised cache for urql that does have an exchange that's implemented correctly (to my knowledge 😆) It's not quite done yet, but probably a nice piece of code to compare the default cache exchange with. https://github.com/kitten/urql-exchange-graphcache/blob/master/src/exchange.ts

I'll post a comment here when I get to this bug, but please let me know if you're interested in opening a PR for a fix, which would be amazing 💯

[federicobadini]

Sorry for the delay. They were very busy days. 😆
I've opened a PR with a proposal to solve this bug. Let me know what you think

[kitten]

No worries! Super happy to have you have helped out here, since we're currently struggling to find enough time to solve the last remaining issues 💯