Don't cache rejected promises - fixes #8

[SamVerschueren]

This PR fixes #8 by not caching a rejected promise by default. I also added a cacheRejection option (couldn't come up with a better name) to re-enable it.

Feedback welcome!

[kevva]

I like cacheRejection or cacheRejected.

[sindresorhus]

This is duplicated. Can you extract it into an arrow function above the if statement on line 36?

[sindresorhus]

I think Set to true if you want to is moot. The default is defined, so we can just describe what it does:

Cache rejected promises.

[sindresorhus]

This can be simplified by using an async function:

const memoized = m(async () => {
	i++;

	if (i === 1) {
		throw new Error('foo bar');
	}

	return i;
}, {
	cacheRejection: true
});

[SamVerschueren]

Much better :)

[sindresorhus]

Let's go with cachePromiseRejection as cacheRejection can sound like it's about the rejection of the cache, not caching of the rejected promise.

[SamVerschueren]

Updated

[sindresorhus]

Perfect 👌🌟

[LinusU]

Hmm, it seems like this will wait to cache the value until the promise have settled, which is not how this module used to behave. I'm currently using the module something like this:

const getPlanInfo = mem(async (planName) => {
  const input = await expensiveApiCallThatFetchesPlanInfo()

  return { currency: input.currency.toUpperCase(), amount: input.amount }
}, { maxAge: A_FEW_HOURS })

In 1.x every request for a specific plan would all return the same Promise and only one request would go out to the remote server. In 2.x there will be one request sent per call until the first Promise settles.

p-memoize works by removing the Promise from the cache if the promise rejects (actually, it seems like it clears the entire cache when any promise rejects, since there is no way to clear a specific entry from the cache), which lets all the first requests use the single outgoing request to my third party service.

---

Would a PR to switch to returning the same pending Promise for all subsequent call until the promise settles be accepted?

[sindresorhus]

Would a PR to switch to returning the same pending Promise for all subsequent call until the promise settles be accepted?

Yes. This was just an oversight.

[LinusU]

Great 👍

There is one small problem though 🤔

There is currently no specification that the provided cache must support .delete, even .clear is optional.

How do you want to solve this, should we add that it should provide .delete (which Map already does)?

edit: quick-lru also supports .delete so that's good 👍

[LinusU]

Submitted #12 which requires .delete to be present on the cache object

[LinusU]

ping @sindresorhus 😄