Skip to content

TypeScript - Fix the cache type #29

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

Merged
merged 6 commits into from
Mar 31, 2019

Conversation

dylang
Copy link
Contributor

@dylang dylang commented Mar 14, 2019

@sindresorhus Thanks for adding types to your repos!

I believe the cache type needs to include the data, otherwise this isn't possible:

const cache = new QuickLRU<string, { data: Bundle }>({
    maxSize: BUNDLE_COUNT_TO_MEMORIZE
});

const loadJsonFileMemorized = mem(
    (filename: string) => loadJsonFile<Bundle>(filename),
    {
        cache
    }
);

// After modifying we want to update the cache directly to avoid another load.
cache.set(filename, { data: updatedBundle });

index.d.ts Outdated
@@ -33,7 +33,7 @@ export interface Options<
*
* @default new Map()
*/
readonly cache?: CacheStorage<CacheKeyType, ReturnType>;
readonly cache?: CacheStorage<CacheKeyType, { data: ReturnType; maxAge?: number; }>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is maxAge optional? It is not being set conditionally in the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you aren't using the maxAge option then this value is not necessary.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this definition, you're basically saying that maxAge may be sometimes provided to the underlying API. This is, however, not the case. maxAge is always set, disregarding of whether you use it or not.

When you implement a custom cache storage you don't actually have to use the maxAge value, it is however, always there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, thanks for that explanation @BendingBender! Changing maxAge to required.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@sindresorhus sindresorhus changed the title fix cache type: { data: ReturnType } TypeScript - Fix the cache type Mar 31, 2019
@sindresorhus sindresorhus merged commit 298a71e into sindresorhus:master Mar 31, 2019
@dylang dylang deleted the fix-cache-type branch March 31, 2019 19:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants