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

Use Keyv type for KeyvMulti.Options stores #211

Merged
merged 1 commit into from
Jul 30, 2024

Conversation

lucasadrianof
Copy link
Contributor

Closes #210.

As described on #210, the remote and local types were incorrectly set to a JS Map (used by Keyv core under the hood). These types should be referencing Keyv instead.

I also fixed the definition unit tests to pass a new Keyv instance for each store to reflect these changes.

A Keyv instance is used by default in the Multi class's constructor in case no override is set (not a Map): https://github.com/microlinkhq/keyvhq/blob/master/packages/multi/src/index.js#L6

The "remote" and "local" types were incorrectly set to a JS Map (which
Keyv uses under the hood). With these changes consuming apps can pass
any Keyv store to the Multi class constructor
@Kikobeats
Copy link
Member

Thanks a lot!

@Kikobeats Kikobeats merged commit ff20ce7 into microlinkhq:master Jul 30, 2024
2 checks passed
@coveralls
Copy link

coveralls commented Jul 30, 2024

Pull Request Test Coverage Report for Build 10151002379

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.2%) to 96.352%

Files with Coverage Reduction New Missed Lines %
packages/redis/src/index.js 2 96.3%
Totals Coverage Status
Change from base Build 9203837957: -0.2%
Covered Lines: 896
Relevant Lines: 916

💛 - Coveralls

@lucasadrianof lucasadrianof deleted the fix/multi-options branch July 30, 2024 20:13
@Kikobeats
Copy link
Member

@lucasadrianof btw what are you building? 😆

@lucasadrianof
Copy link
Contributor Author

@lucasadrianof btw what are you building? 😆

Nothing too crazy 😆

We have some data that is cached on Redis since it's very frequently read. Since we work with k8s, we can't use a local cache only, but now we have implemented a dual cache (local first + fallback to Redis) to decrease the latency on these reads even more (with a local cache per pod first).

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.

KeyvMulti.Options has an incorrect type signature
3 participants