-
Notifications
You must be signed in to change notification settings - Fork 214
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
Cache is global, can potentially conflict with external modules #41
Comments
I think this is a matter of personal opinion. There are other node caches that do this, but then they suffer from not following typical require patterns (requiring in multiple places does not duplicate itself). Why not instead namespace your cache keys? |
@brandonculver that still wouldn't work. Let's say I have a library which exposes a SomeClient class which uses the node-cache module internally. var SomeClient = require('my-module').SomeClient;
var client1 = new SomeClient();
var client2 = new SomeClient(); Both client1 and client2 will be using the same cache. This makes node-cache fine to use for "standalone" apps but restricts its use for published libraries. |
@olalonde I understand what you are saying. I mean namspacing your cache keys. ('myapp-cache1', myapp-cach2) I would also question why you are using this particular module in other libraries. Its a pretty bare bones cache solution. Again, there are other caching solutions that do exactly what you want https://www.npmjs.com/package/node-cache It just seems like unnecessary overhead in a currently simple module. Ultimately its not my call though 😉 |
Oh thanks I wasn't aware of this module! FWIW, I don't think it's unnecessary overhead. In fact, I just implemented this feature in ~4 lines of code :). Plus it's backwards compatible and doesn't force anyone to use it. |
+1。namespace key is a workaround friendly for mistakes, for example, some module also use memory cache and finally call
then all my module's cache is away. |
What happens when running in cluster mode, like pm2 does? Could this mean the cache is shared between threads? Edit no it doesn't, I've just verified this. |
Since required modules are cached in Node.js, it means that multiple modules could be using the same cache at the same time. There should be a
.createStore()
method that creates a new cache instance.The text was updated successfully, but these errors were encountered: