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

Add onCacheError option #646

Merged
merged 10 commits into from Apr 4, 2024

Conversation

slukes
Copy link
Contributor

@slukes slukes commented Mar 3, 2024

Please check if the PR fulfills these requirements

  • Followed the Contributing guidelines.

  • Tests for the changes have been added (for bug fixes/features) with 100% code coverage.

  • Docs have been added / updated (for bug fixes / features)

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

This PR addresses:

#643
#588
#51

By re-adding an option that existed in caching.js to 'ignore' cache errors but was removed during the migration to typescript / promises.

This is useful to applications that are using a distant cache store, to enable to application to continue to work, even when the cache backend is down.

@slukes slukes marked this pull request as ready for review March 3, 2024 21:09
@jaredwray
Copy link
Owner

@slukes - I would rather not have another config on this and since it is cache should we just add in EventEmitter3 and emit an error when it goes to a catch?

@slukes
Copy link
Contributor Author

slukes commented Mar 10, 2024

@slukes - I would rather not have another config on this and since it is cache should we just add in EventEmitter3 and emit an error when it goes to a catch?

Hello @jaredwray I'm not sure how this is going to work! I've created a ticket on our team backlog to dig into this further in the next couple of weeks

@jaredwray
Copy link
Owner

@slukes - what I am saying is that instead of doing an ignoreErrors property in the cache manager what we can do is enable you to do this instead:

import { caching } from 'cache-manager';

const cache = await caching('memory', {
  max: 100,
  ttl: 10 * 1000 /*milliseconds*/,
});

cache.on('error', (error) = > {
     console.log(error);
});

This would use events to emit an error and by default we would make all methods catch errors and not throw. This gives the resiliency shown in keyv that many love.

Would that work for you?

src/multi-caching.ts Outdated Show resolved Hide resolved
src/caching.ts Show resolved Hide resolved
test/caching.test.ts Outdated Show resolved Hide resolved
test/caching.test.ts Outdated Show resolved Hide resolved
test/multi-caching.test.ts Outdated Show resolved Hide resolved
@slukes slukes force-pushed the add-ignore-cache-errors-option branch from f75d021 to c0132e7 Compare April 3, 2024 15:23
@slukes
Copy link
Contributor Author

slukes commented Apr 3, 2024

@jaredwray thanks to the work of @AdrienDoctolib we now have something that is working how you suggested. Please let us know your feedback.

We have tested in our application and all is well 👍

@jaredwray
Copy link
Owner

@AdrienDoctolib and @slukes looks like we are getting an error on compile time:

Screenshot 2024-04-03 at 11 09 29 AM

@slukes
Copy link
Contributor Author

slukes commented Apr 3, 2024

@AdrienDoctolib and @slukes looks like we are getting an error on compile time:

Sorry about that - fixed!

@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 90.90909% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 98.96%. Comparing base (511e426) to head (140864b).

Files Patch % Lines
src/multi-caching.ts 80.00% 4 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@             Coverage Diff             @@
##              main     #646      +/-   ##
===========================================
- Coverage   100.00%   98.96%   -1.04%     
===========================================
  Files            5        5              
  Lines          365      387      +22     
  Branches        89       93       +4     
===========================================
+ Hits           365      383      +18     
- Misses           0        4       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@slukes slukes requested a review from jaredwray April 4, 2024 12:34
@jaredwray jaredwray merged commit b03c2ad into jaredwray:main Apr 4, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants