feat: [GH-177] Custom Cache Set Options

[HishamAli81]

To be able to support custom key value caches that have additional cache set options, add an optional generic argument KeyValueCache to be able to specify a custom cache set option type.

Related to issue: apollographql/datasource-rest#177

[changeset-bot]

🦋 Changeset detected

Latest commit: 3b6d6ba

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages

Name	Type
@apollo/utils.keyvaluecache	Minor
@apollo/utils.keyvadapter	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

[trevor-scheer]

I think it's good to add this noUpdateTTL test, but I don't think we should stop testing the ttl: null case. There should be two tests here.

[trevor-scheer]

Actually it looks like ttl: null is no longer allowed (but ttl: 0 is, and the documentation on ttl says 0 is forever). We should update this test to ttl: 0 and update the test description.

[HishamAli81]

@trevor-scheer Thanks for catching this. Yeah, since LRUCache.SetOptions.ttl doesn't support null, I had to update the test, but admitted didn't do a good job of actually checking what the test was asserting 😞

I can just change it to ttl: 0 as suggested since that's what LRUCache.SetOptions.ttl supports, or another option would be to override the ttl definition to support null:

export type InMemoryLRUCacheSetOptions<
  V extends {} = string,
  FC = unknown,
> = Omit<LRUCache.SetOptions<string, V, FC>, "ttl"> & KeyValueCacheSetOptions;

And then always default to ttl: 0 if it's falsy within the InMemoryLRUCache.set() function:

async set(key: string, value: V, options?: SO) {
  // If TTL is defined, convert it from seconds to milliseconds.
  // Otherwise default it to 0 to indicate "no TTL".
  this.cache.set(key, value, {
    ...options,
    ttl: options?.ttl ? options.ttl * 1000 : 0,
  });
}

This way, we're not breaking the current interface and would still be able to support ttl: null as KeyValueCacheSetOptions does.

Do you have a preference between the 2 approaches?

[HishamAli81]

@trevor-scheer I opted to be backwards compatible and reverted back to be able to support a null TTL value. Let me know what you think.

[trevor-scheer]

I'm not sure 0 is the right thing to do here.

The docs say if it's set at all, it needs to be a positive number. So if this works, it's undocumented behavior. I would prefer this to remain omitted if not set.

[HishamAli81]

@trevor-scheer According to the TTL type definition, a TTL of 0 indicates "no TTL":

/**
     * Max time in milliseconds for items to live in cache before they are
     * considered stale.  Note that stale items are NOT preemptively removed
     * by default, and MAY live in the cache long after they have expired.
     *
     * Also, as this cache is optimized for LRU/MRU operations, some of
     * the staleness/TTL checks will reduce performance, as they will incur
     * overhead by deleting items.
     *
     * Must be an integer number of ms. If set to 0, this indicates "no TTL"
     *
     * @default 0
     */
    ttl?: Milliseconds

And the TTL is also defaulted to 0 if not specified: https:/isaacs/node-lru-cache/blob/main/src/index.ts#L1124

I don't necessarily disagree that it might be best for the lru-cache library to apply the default TTL instead of defaulting the TTL here, but the issue I ran into in that I'm unable to set the value to undefined because of the "exactOptionalPropertyTypes": true config in the tsconfig.base.json. So I'd need to adjust the tsconfig as well if I need to set the TTL to undefined.

Also do note, that a TTL value of null is accepted by KeyValueCacheSetOptions but isn't by LRUCache.SetOptions, so the null TTL would need to be converted to undefined (or 0).

[trevor-scheer]

Interesting, good catch. Opened an issue to see if there's a README update needed.
isaacs/node-lru-cache#316