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

Throw on clear(fn) if fn’s cache can't be cleared #59

Merged
merged 1 commit into from
Oct 7, 2020

Conversation

fregante
Copy link
Collaborator

@fregante fregante commented May 16, 2020

Breaking change, to be released after #58 (which is a patch for the current version)

What's the point of calling clear(fn) if it does nothing? This just causes silent errors.

Regarding throwing errors:

  • I can drop the explicit Can't clear a function that was not memoized! error since now it throws anyway, OR
  • I can add an unclearable cache error

@fregante fregante marked this pull request as ready for review May 16, 2020 20:06
@sindresorhus
Copy link
Owner

When would cacheStore.get(fn).clear(); throw?

@fregante
Copy link
Collaborator Author

fregante commented May 17, 2020

It throws if fn is missing: cacheStore.get(fn) is undefined because it’s undefined.clear

It throws if fn’s Map doesn’t have clear: cacheStore.get(fn).clear is not a function

@sindresorhus
Copy link
Owner

I can add an unclearable cache error

👍

@sindresorhus
Copy link
Owner

Is it really worth doing another breaking release just for this? I suggest releasing what's in master as a minor release and then merging this, but not release it until we need to do another release.

@fregante
Copy link
Collaborator Author

You could also keep this PR open until there’s another reason to release a breaking version.

@sindresorhus
Copy link
Owner

Yeah, we can do that.

This was referenced Sep 29, 2020
@fregante fregante marked this pull request as draft October 3, 2020 08:12
@fregante fregante added the bug label Oct 6, 2020
@fregante fregante marked this pull request as ready for review October 6, 2020 06:59
@fregante
Copy link
Collaborator Author

fregante commented Oct 6, 2020

I can add an unclearable cache error

I pushed a change based on the latest version and added a test. This is now ready to be merged

@sindresorhus sindresorhus merged commit e7c8893 into master Oct 7, 2020
@sindresorhus sindresorhus deleted the throw-with-clear branch October 7, 2020 12:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants