-
-
Notifications
You must be signed in to change notification settings - Fork 351
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
Why does the fetch function return void ? #302
Comments
I just run into the same issue. Worst case scenario it should be |
If the fetch is aborted due to the key being evicted or deleted prior to the fetchMethod returning, then it might not return a value. It's an edge case, but it is possible, and the previous declared type was incorrect by not including it. For full type safety, test that the return type is the type you want, and handle the error case. If you know that can't happen and wanna yolo it, you can coerce it with an explicit |
Indeed that is what I've done in my case but to be fair it doesn't seem like a really good practice. If you Lru-cache change the behavior (and the returned types) of the fetch method I won't be warned. Could returning |
Well, the issue is that the promise might resolve without a value if the fetchMethod doesn't return a value when it's aborted. Which, there's no reason to require it to return What problem does it cause to have At the very least, removing the Here's an example: import { LRUCache } from './'
const c = new LRUCache<number, string>({
max: 3,
fetchMethod: async (key, _oldValue, { signal }) => {
await new Promise<void>(res => setTimeout(() => res(), 100))
if (signal.aborted) {
console.error('aborted, returning void', key)
// this would have to be `return undefined`, whereas `return` is allowed now,
// because fetch() resolves to the promise that the fetchMethod returns.
return
}
return String(key)
},
ignoreFetchAbort: true,
allowStaleOnFetchAbort: true,
})
Promise.all([
c.fetch(1),
c.fetch(2),
c.fetch(3),
c.fetch(4),
]).then(console.log) $ ts-node fetch-void.ts
aborted, returning void 1
[ undefined, '2', '3', '4' ] |
That's just an issue when writing pure TS but I guess that Monkey patch - We don't want that fetch return
|
I write pure TS, but what's the issue, exactly? What situation are you using it in, and what error occurs? I'm ok with making it undefined instead of void, just want to understand why. The reason I'm pushing back a bit on this is that it means changing the type signature of a public method in an incompatible way, so it's a SemVer breaking change, as it may require downstream users to update their code. |
I hit the issue working on this PR for an open-source project. I ended up doing the yolo mode (coercing it with an explicit If we don't use the explicit cast. We can't use the
|
According to the TS documentation:
The The problem with void when using this function is that TS does not treat it as if it was undefined, even if in JS there is no difference. So in this case |
@ghusse It doesn't return actual |
Hey there. I see the reason for having Below is an example of a simple wrapper which takes a function, initialises a cache and returns a cached version of the input function (same signature). Upon export function memoizeLru<Fn extends (...args: Args) => Promise<Result>, Args extends any[], Result extends object>(
fn: Fn,
options:
| Omit<LRUCache.OptionsMaxLimit<string, Result, { args: Args }>, 'fetchMethod'>
| Omit<LRUCache.OptionsTTLLimit<string, Result, { args: Args }>, 'fetchMethod'>,
resolveKey: (...args: Args) => string
): (...args: Args) => Promise<Result> {
const cache = new LRUCache<string, Result, { args: Args }>({
...options,
fetchMethod: async (_key, _staleValue, { context }) => {
return await fn(...context.args);
}
});
return async (...args: Args) => {
const result = await cache.fetch(resolveKey(...args), { context: { args } });
if (!(result instanceof Object)) {
// result might be of type void (in case of abort)
// should never happen here (we use no signals)
throw new Error('fetch aborted');
}
return result;
};
} Hey, and thanks for this great lib ❤️ |
The TS doc is pretty clear about the usage of void : as soon as a function CAN return something, void should not be used |
@ghusse There is a difference between returning |
Ok, so, semantic details about what specifically The point is, It's not super hard to make it It'll be fixed in version 10. I've been dragging my feet just because I wanted to make sure it's worth the disruption when bumping major on a project like this, and also because I'm feeling lazy, but I guess it's time to update all of my projects again lol. |
👍 thanks a lot for the update |
Switched to |
Why doesn't |
@alexgleason In many cases, it does throw (well, reject) when the fetch is aborted. But |
Or, for example, if you do |
Hi,
I'm on version
^9.1.1
I try to use the
fetch()
function with Typescript. However, the return type isvoid | V
. I expect it to return only the Value type only. Thevoid
type will break some of my TS.Version
7.18
return the value only without voidThe text was updated successfully, but these errors were encountered: