Change the default value of the cachePromiseRejection option to true

[fregante]

As a feature, this already feels weird because caching depends on this specific mechanism and not others like:

• should the function be cached if it returns false?
• should the function be cached if its call parameters include {cache: false}?

Having it avoid cache just because a promise is rejected feels arbitrary... but in a way I see that it's a flexible mechanism that allows any function (sync or async) to decide whether to avoid the cache.

That said, I don't think it should be a default, because:

• there's a chance that if something errors the first time, it will also error the second time (idempotency is a virtue);
• seen as a mechanism to avoid cache, it should be opt-in if needed, rather than an opaque behavior the developer didn't ask for.

[sindresorhus]

I can see arguments for having the default both true and false. Personally, the default I would expect is true. I'll need to think about this more. Would be useful with input from some more people.

// @LinusU

[szmarczak]

I think it should be set to true because most async function aren't dynamic like Got. Decompressing content? Calculating big numbers? Sure. Downloading? Nah. So, if someone wanted to use it with Got then he would need to disable that.

[LinusU]

This is a hard one, there are definitely arguments for both sides, but I'm personally leaning against true as well.

[szmarczak]

@LinusU Can you elaborate on why?

[timdp]

I was unaware of this behavior until Sindre tweeted about it.

I really don't see what memoization has to do with promises altogether. Therefore, I also don't see why the option even exists, let alone why it has an unintuitive default.

At most, I would expect if from a hypothetical p-mem package. Even there, the default should be the opposite.

If you keep this behavior, please make it more clear in the readme.

[fregante]

I would expect if from a hypothetical p-mem package

That's not a bad point, actually, since https://github.com/sindresorhus/p-memoize exists and is promise-specific. However as I said in my first post:

it's a flexible mechanism that allows any function (sync or async) to decide whether to avoid the cache.

[LinusU]

Can you elaborate on why?

Sorry for the short response 😄

On the one hand, you could argue that since it doesn't catch thrown errors and re-throw them when called later, it shouldn't do "the same" for async functions.

That being said, I think that it becomes quite "magic" when it removes Promises when they reject, since that happens at some point later and it then has to go in and remove the stored promise. This makes it quite a difference compared to a throwing function.

It just seems like which behavior you want is very usage-dependent, and it's impossible to pick a perfect default. Therefore I think that the less "magic" way should be the default, as not to accidentally opt people into something they might not be aware of. Just my 2¢ ☺️

I really don't see what memoization has to do with promises altogether. Therefore, I also don't see why the option even exists, let alone why it has an unintuitive default.

I also agree with this, I think that this package shouldn't deal with promises at all, since we have p-memoize for that.

[sindresorhus]

Thanks everyone for sharing useful feedback. 🙌 Since we're pretty split on what the default should be, I buy the argument about doing the least magic/surprising thing by default.

[sindresorhus]

I also agree with this, I think that this package shouldn't deal with promises at all, since we have p-memoize for that.

Yeah, maybe, but I'll keep it for now as p-memoize is just a small facade for mem right now. I haven't had the time to make it focused on just the async case.