sindresorhus / p-memoize

Memoize promise-returning & async functions
MIT License
396 stars 30 forks source link

Allow `.set()` to be async, only cache promises until they resolve, synchronously cache pending promises and remove `cachePromiseRejection` #48

Closed Richienb closed 2 years ago

Richienb commented 2 years ago

Before, if a function is called twice before the asynchronous cache is checked in the first call, the second call will not recognise the first one. Now, instead of the promise returned from the memoized function being cached, the promise from the function that calls the memoized function is cached.

Makes the function more predictable, though there will always be edge cases and race conditions.

cachePromiseRejection is removed because it only happens locally and to have it actually affect the main cache would require us to create a one-size-fits-all solution to serialize errors and a custom storage format to represent fulfilments and rejections. See https://github.com/sindresorhus/p-memoize/pull/47#issuecomment-1153673694

36 suggests to make all cache methods asynchronous. What do you think?

Fixes #47, Fixes #34

mdawar commented 2 years ago

https://github.com/sindresorhus/p-memoize/pull/36 suggests to make all cache methods asynchronous. What do you think?

I think you can safely await cache methods even if they're not async.

See await:

If the value of the expression following the await operator is not a Promise, it's converted to a resolved Promise.

Richienb commented 2 years ago

TypeScript wants us to explicitly declare whether we're going to allow them to return promises.

Richienb commented 2 years ago

Nevermind, the real problem is pMemoizeClear is a synchronous function. If we want to allow .clear to be asynchronous, we'll need to make this function asynchronous too.

wyntau commented 2 years ago

will this change be ported to version 4.x?

Richienb commented 2 years ago

will this change be ported to version 4.x?

No, it is technically a breaking change.

Richienb commented 2 years ago

@sindresorhus

wyntau commented 2 years ago

will this change be ported to version 4.x?

No, it is technically a breaking change.

We are using version 4.x. When I see this PR, I think the version 4.x may not reuse the pending promise too. After I checked the source code, I found 4.x will reuse the pending promise if called memoized fn with the same params twice.

So I have no problem now :-)

Thanks.

const memoized = async function (...arguments_) {
  const key = cacheKey ? cacheKey(arguments_) : arguments_[0];

  const cacheItem = cache.get(key);
  if (cacheItem) {
    return cacheItem.data;
  }

  const promise = fn.apply(this, arguments_);
  cache.set(key, {
    data: promise,
    // Ref. https://github.com/sindresorhus/p-memoize/issues/39#issuecomment-985301556
    maxAge: (2 ** 31) - 1
  });

  const [{reason}] = await pSettle([promise]);
  if (!cachePromiseRejection && reason) {
    cache.delete(key);
  } else if (maxAge) {
    // Promise fulfilled, so start the timer
    cache.set(key, {
      data: promise,
      maxAge: Date.now() + maxAge
    });
  }

  return promise;
};