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

An option to wait for cache.set to resolve before resolving the returned promise (or even make this the default behavior) #34

Closed
futpib opened this issue Nov 7, 2021 · 4 comments · Fixed by #48

Comments

@futpib
Copy link

futpib commented Nov 7, 2021

One can bypass cache by making multiple consecutive calls with the same arguments:

// Imagine a custom `cache` with a slow `cache.set` and a fast `cache.has`
await memoized('foo'); // Calls `cache.set('foo', 'bar')`
// First `cache.set('foo', 'bar')` promise did not resolve yet and `cache.has('foo')` will resolve to `false`
await memoized('foo'); // Calls `cache.set('foo', 'bar')` again
// Some time later...
// First `cache.set('foo', 'bar')` promise resolved
// Only now will `cache.has('foo')` resolve to `true`
// Second `cache.set('foo', 'bar')` promise resolved but it should not have been called in the first place

cache.set(key, result);

See this PR for a test that demonstrates this #35
See this PR for a head-on fix (breaking change) that awaits all cache operations #36

futpib added a commit to futpib/p-memoize that referenced this issue Nov 7, 2021
futpib added a commit to futpib/p-memoize that referenced this issue Nov 7, 2021
futpib added a commit to futpib/p-memoize that referenced this issue Nov 7, 2021
@Richienb
Copy link
Contributor

Richienb commented Nov 7, 2021

@sindresorhus

Maybe it should be made an option?

@Richienb
Copy link
Contributor

Richienb commented Nov 7, 2021

Even if we did await all set calls, behold this simple example:

await Promise.all([
	memoized('foo'),
	memoized('foo'),
]);

@sindresorhus
Copy link
Owner

^ @futpib Any opinion?

It's unclear to me what the best way forward here is.

@Richienb
Copy link
Contributor

@sindresorhus Solved it by caching the entire asynchronous part of the pMemoize function rather than just the promise returned by the memoized function. See #48.

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