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.55k stars 443 forks source link

request-policy-exchange behavior differs between dev & production #3462

Closed Akkuma closed 5 months ago

Akkuma commented 7 months ago

Describe the bug

The exchange has this basic logic:

const processIncomingResults = (result: OperationResult): void => {
  const meta = result.operation.context.meta;
  const isMiss = !meta || meta.cacheOutcome === 'miss';
  if (isMiss) {
    operations.set(result.operation.key, new Date().getTime());
  }
};

meta does not exist in production (as mentioned https://github.com/urql-graphql/urql/discussions/3439) resulting in this always being viewed as a miss. This will keep a request alive much longer than anticipated and won't be the same behavior between dev and production.

Reproduction

Is this needed here?

Urql version

4.0.6, core 4.2.2

Validations

kitten commented 7 months ago

No reproduction needed here indeed! Nice catch. We haven't kept track of this exchange as much as we should have, so i'll get on that and will see what we can do.

It might be worth evaluating at the same time whether this exchange should be expanded to include more logic.

Akkuma commented 7 months ago

No reproduction needed here indeed! Nice catch. We haven't kept track of this exchange as much as we should have, so i'll get on that and will see what we can do.

It might be worth evaluating at the same time whether this exchange should be expanded to include more logic.

Locally I modified this exchange to use localStorage rather than be in memory, so that the ttl persists across multiple sessions. What I've done locally is remove a key when it should be upgraded and my set will only set if there is no key. This keeps it from impacting the intended ttl and doesn't require meta to exist either:

const get = (key: string | number): number => {
    const val = localStorage.getItem(`${storageNamespace}${key}`);
    return val ? parseInt(val, 10) : 0;
};

const set = (key: string | number): void => {
    if (localStorage.getItem(`${storageNamespace}${key}`)) return;

    localStorage.setItem(`${storageNamespace}${key}`, `${Date.now()}`);
};

const remove = (key: string | number): void => {
    localStorage.removeItem(`${storageNamespace}${key}`);
};

// relevant get & remove code
const currentTime = Date.now();
const lastOccurrence = get(operation.key);
if (currentTime - lastOccurrence > TTL && (!options.shouldUpgrade || options.shouldUpgrade(operation))) {
  remove(operation.key);
  return makeOperation(operation.kind, operation, {
    ...operation.context,
    requestPolicy: 'cache-and-network',
  });
} 

I could open up a PR to support an optional storage get/set/remove like the above allowing for any storage mechanism.