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

Handle exceptions thrown by cache reads #37

Merged
merged 2 commits into from
Jul 22, 2020
Merged

Conversation

jordanholt
Copy link
Contributor

Description

Handle exceptions thrown by attempted async calls to store.set().

Motivation and Context

Prevent cache engine issues resulting in an unhandled exception bubbling up to an EUNKNOWN error.

How Has This Been Tested?

Manual testing.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@jordanholt jordanholt requested a review from a team as a code owner July 21, 2020 13:13
Copy link

@crosslandwa crosslandwa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OBSERVATION: I think this needs documenting (via tests and/or the README.md)

This implementation means if the supplied cache implementation is "broken" the library will never actually try to make outgoing requests - I agree with this behaviour, but feels its a design decision that should be documented

edit
on further consideration, i've realised this doesn't change the above behaviour (it was already throwing/Promise.rejecting if the supplied cache is faulty) - its now giving an easier to reason about error.

Changing to an APPROVE (though some unit tests in caching.test.ts might still be worthwhile)

@jordanholt
Copy link
Contributor Author

👍 I'll add some more unit tests, that file is the only one with <75% branch coverage.

Copy link
Contributor

@ben-whitfield ben-whitfield left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jordanholt jordanholt merged commit 522e625 into master Jul 22, 2020
@jordanholt jordanholt deleted the cache-read-exception branch July 22, 2020 15:09
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.

4 participants