Skip to content
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

Warn when cacheKey is unset and function.length > 1? #90

Open
fregante opened this issue Feb 1, 2023 · 6 comments
Open

Warn when cacheKey is unset and function.length > 1? #90

fregante opened this issue Feb 1, 2023 · 6 comments

Comments

@fregante
Copy link
Collaborator

fregante commented Feb 1, 2023

By default, only the memoized function's first argument is considered via strict equality comparison

I think mem should call console.warn in this case:

mem(async (host, path) => fetch(host + path))

Correct usage:

mem(async (host, path) => fetch(host + path), {
  cacheKey: args => args.join()
})
mem(async (host, path) => fetch(host + path), {
  cacheKey: JSON.stringify
})
@sindresorhus
Copy link
Owner

What if the parameter is there but does not change, so does not affect memoization? I don't think we can reliably detect incorrect usage here. And console.warn is not really something you want to be using in a reusable package.

@fregante
Copy link
Collaborator Author

fregante commented Feb 1, 2023

What if the parameter is there but does not change

I think you're talking about, say, a function that accepts a (string, opts), where opts is always omitted by the caller, thus "never changes".

What the caller does with the function is irrelevant. Since memoization wraps the function, it should follow the function’s logic, not the caller’s. e.g.

export const normal = fetch;
export const memoized = mem(fetch);

If a function accepts a parameter, why would it "not change"? If that exists, it sounds like an exception.

And console.warn is not really something you want to be using in a reusable package

Agreed, but it sounds like a good warning and it's easy to silence:

console.warn(`
Function accepts 2 parameters, but only one is considered during mem.
Use \`cacheKey: ([x]) => x\` to silence this message or refer to the docs:
https://github.com/sindresorhus/mem#caching-strategy
`)
  mem(async (host, path) => fetch(host + path), {
+    cacheKey: ([x]) => x
  })

@sindresorhus
Copy link
Owner

If a function accepts a parameter, why would it "not change"? If that exists, it sounds like an exception.

For example, an API with a parameter where you always pass in the same value as you only use it that way. Yes, the correct way would be to set a custom cache key, but since the parameter is practically static, it feels moot.

@sindresorhus
Copy link
Owner

I'm ok with console.warn with a good message like the above.

@fregante
Copy link
Collaborator Author

fregante commented Feb 4, 2023

Side quest: detecting functions with optional parameters/rest.

(url, opts = {}) => {} would not be detected by .length. It would be interesting to detect the real length. For rest parameters, the mere existence should trigger the warning;

await memQuantumSum(12, 45, 71)

I think however this requires a whole JS parser though 😰 so, no.

@fregante
Copy link
Collaborator Author

fregante commented Feb 4, 2023

Wait a minute. TypeScript to the rescue? This is right up its alley and better than a runtime warning.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants