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

Change the default value of the `cachePromiseRejection` option to `true` #36

Merged
merged 4 commits into from May 17, 2019

Conversation

Projects
None yet
5 participants
@bfred-it
Copy link
Collaborator

commented May 10, 2019

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.

@bfred-it bfred-it referenced this pull request May 11, 2019

Closed

Drop `maxAge` #37

@sindresorhus

This comment has been minimized.

Copy link
Owner

commented May 13, 2019

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

Show resolved Hide resolved readme.md
@szmarczak

This comment has been minimized.

Copy link

commented May 13, 2019

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 comment has been minimized.

Copy link
Contributor

commented May 14, 2019

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

@szmarczak

This comment has been minimized.

Copy link

commented May 14, 2019

@LinusU Can you elaborate on why?

@timdp

This comment has been minimized.

Copy link

commented May 14, 2019

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.

@bfred-it

This comment has been minimized.

Copy link
Collaborator Author

commented May 15, 2019

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

This comment has been minimized.

Copy link
Contributor

commented May 15, 2019

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

This comment has been minimized.

Copy link
Owner

commented May 17, 2019

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

This comment has been minimized.

Copy link
Owner

commented May 17, 2019

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.

@sindresorhus sindresorhus changed the title Default to caching rejected promises Change the default value of the `cachePromiseRejection` option to `true` May 17, 2019

@sindresorhus sindresorhus merged commit 70707ae into sindresorhus:master May 17, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@bfred-it bfred-it deleted the bfred-it:patch-2 branch May 17, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.