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

feat: support sync cache creation #598

Merged
merged 4 commits into from Dec 18, 2023
Merged

Conversation

denkan
Copy link
Contributor

@denkan denkan commented Sep 29, 2023

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, ...)

Add non-async createCache() that caching() uses to set up methods (keep current functionality intact).

Having createCache() as addition to caching() allows devs to create cache instances synchronously, and i.e use as default params in methods:

/** Awesome service that will use memory store as default cache, if nothing else specified */ 
class MyItemService {
   constructor(public cache = createCache(memoryStore())) { }
}

Copy link
Contributor

@douglascayers douglascayers left a comment

Choose a reason for hiding this comment

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

I'm a consumer of this lib like you, and for what it's worth, your changes look good to me, thanks!

We recently migrated from v4 to v5 and were a bit surprised that the method to create a cache became async. Your change will make those migrations easier, plus the benefits of when needing to instantiate the cache in a sync environment (e.g. constructors)

@jaredwray
Copy link
Owner

@denkan - can you add in an update to the readme on how to use this?

@denkan
Copy link
Contributor Author

denkan commented Oct 10, 2023

@denkan - can you add in an update to the readme on how to use this?

Updated readme with short section about createCache() / synchronous creation.

@codecov-commenter
Copy link

codecov-commenter commented Oct 11, 2023

Codecov Report

Merging #598 (bb1ddda) into master (66246e9) will not change coverage.
The diff coverage is 100.00%.

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

@@            Coverage Diff            @@
##            master      #598   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            5         5           
  Lines          292       313   +21     
  Branches        75        77    +2     
=========================================
+ Hits           292       313   +21     
Files Coverage Δ
src/caching.ts 100.00% <100.00%> (ø)

@jaredwray
Copy link
Owner

@denkan - thanks so much for doing this. One thing I was thinking is that we might want to change it to be createMemoryCache as a simple helper function. The reason is that it could be confusing to do this function as sync when if they put in an async store it will cause issues.

You could even simplify and not require them to put in the in-memory store if you wanted.

Can you change that over and update the branch to latest? If you are good with these changes and update it we can check this in. 🎉

@casey-chow
Copy link

One thing I was thinking is that we might want to change it to be createMemoryCache as a simple helper function. The reason is that it could be confusing to do this function as sync when if they put in an async store it will cause issues.

The main point of this PR is to allow synchronous creation of a cache as opposed to creation of synchronous cache. That's to say, even if I were using redis, I would still want that to be a synchronous cache since some redis drivers will lazy connect on first access. The main use case here is that it's quite annoying to have to write things like await (await cache).get('key') rather than just await cache.get('key').

@alfonsograziano
Copy link

I'd love to use this feature as well!

@jaredwray
Copy link
Owner

@casey-chow - we are ready to merge this. Can you do the following:

  • update your branch to the latest and resolve conflicts
  • make sure you are at 100% test coverage
  • make sure documentation with how to use this is updated

🍻

@jaredwray
Copy link
Owner

@casey-chow are you able to update the branch?

Add non-async `createCache()` that `caching()` uses to set up methods.
@denkan
Copy link
Contributor Author

denkan commented Dec 17, 2023

@jaredwray and others,
Sorry I dropped the ball on this one (as I mostly use another github account for daily work nowadays).

✅ I've rebased and resolved conflicts.

@denkan - thanks so much for doing this. One thing I was thinking is that we might want to change it to be createMemoryCache as a simple helper function. The reason is that it could be confusing to do this function as sync when if they put in an async store it will cause issues.

You could even simplify and not require them to put in the in-memory store if you wanted.

I don't really want to do this, as it's not actually limited to memoryStore? I could easily define custom store that doesn't require async initialization.

And I think it's better to let the user of the lib define a shorthand if needed:

export const defaultCache = () => createCache(memoryStore({ /* my options */ })); 

// later
class MyItemService {
   constructor(public cache = defaultCache()) { }
}

Let me know your thoughts or if it's ready to merge 🍻

@jaredwray jaredwray merged commit 0771b7f into jaredwray:master Dec 18, 2023
2 checks passed
garrappachc pushed a commit to tf2pickup-org/server that referenced this pull request Dec 22, 2023
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
|
[cache-manager](https://togithub.com/node-cache-manager/node-cache-manager)
| [`5.3.1` ->
`5.3.2`](https://renovatebot.com/diffs/npm/cache-manager/5.3.1/5.3.2) |
[![age](https://developer.mend.io/api/mc/badges/age/npm/cache-manager/5.3.2?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/cache-manager/5.3.2?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/cache-manager/5.3.1/5.3.2?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/cache-manager/5.3.1/5.3.2?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

### Release Notes

<details>
<summary>node-cache-manager/node-cache-manager (cache-manager)</summary>

###
[`v5.3.2`](https://togithub.com/node-cache-manager/node-cache-manager/releases/tag/v5.3.2)

[Compare
Source](https://togithub.com/node-cache-manager/node-cache-manager/compare/v5.3.1...v5.3.2)

##### What's Changed

- Use a github action workflow to release new versions by
[@&#8203;QuentinLemCode](https://togithub.com/QuentinLemCode) in
[jaredwray/cache-manager#612
- adding in prepare for the build by
[@&#8203;jaredwray](https://togithub.com/jaredwray) in
[jaredwray/cache-manager#618
- fix: exclude sourcemap when package publish by
[@&#8203;czy88840616](https://togithub.com/czy88840616) in
[jaredwray/cache-manager#622
- updating clean to scripts by
[@&#8203;jaredwray](https://togithub.com/jaredwray) in
[jaredwray/cache-manager#623
- feat: support sync cache creation by
[@&#8203;denkan](https://togithub.com/denkan) in
[jaredwray/cache-manager#598
- 619: Ensure that background refresh only calls fn once. by
[@&#8203;ricall](https://togithub.com/ricall) in
[jaredwray/cache-manager#620
- updating typescript and removing release workflow by
[@&#8203;jaredwray](https://togithub.com/jaredwray) in
[jaredwray/cache-manager#624
- updating vitest to 1.1.0 by
[@&#8203;jaredwray](https://togithub.com/jaredwray) in
[jaredwray/cache-manager#625
- upgrading promise-coalesce to 1.1.2 by
[@&#8203;jaredwray](https://togithub.com/jaredwray) in
[jaredwray/cache-manager#626
- upgrading lru-cache to 10.1.0 by
[@&#8203;jaredwray](https://togithub.com/jaredwray) in
[jaredwray/cache-manager#627
- upgrading typescript and eslint to latest by
[@&#8203;jaredwray](https://togithub.com/jaredwray) in
[jaredwray/cache-manager#628

##### New Contributors

- [@&#8203;QuentinLemCode](https://togithub.com/QuentinLemCode) made
their first contribution in
[jaredwray/cache-manager#612
- [@&#8203;czy88840616](https://togithub.com/czy88840616) made their
first contribution in
[jaredwray/cache-manager#622
- [@&#8203;denkan](https://togithub.com/denkan) made their first
contribution in
[jaredwray/cache-manager#598
- [@&#8203;ricall](https://togithub.com/ricall) made their first
contribution in
[jaredwray/cache-manager#620

**Full Changelog**:
jaredwray/cache-manager@v5.3.0...v5.3.2

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://developer.mend.io/github/tf2pickup-org/server).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4xMDMuMSIsInVwZGF0ZWRJblZlciI6IjM3LjEwMy4xIiwidGFyZ2V0QnJhbmNoIjoibWFzdGVyIn0=-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
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

6 participants