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

Fails to cache Promises properly #30

Closed
matthieusieben opened this issue May 29, 2015 · 9 comments
Closed

Fails to cache Promises properly #30

matthieusieben opened this issue May 29, 2015 · 9 comments
Assignees

Comments

@matthieusieben
Copy link

Promises (available in Node 0.12+) should not be cloned (or the clone module should handle Promises properly).
Currently, nodecache breaks implementations caching promises.

@mpneuried
Copy link
Contributor

can you provide a example or test case?

@mpneuried mpneuried self-assigned this May 29, 2015
@matthieusieben
Copy link
Author

var NodeCache = require( "node-cache" );
var myCache = new NodeCache();
var p = new Promise(function(fulfill, reject) { fulfill('Some deferred value'); });

p.then(function(value) {
    console.log('promise result', value);
});

myCache.set('p', p);

var q = myCache.get('p');

try {
    q.then(function(value) {
        console.log('cached promise result', value);
    });
} catch(e) {
    console.error('Unable to treat q as a promise', e);
}

I just saw that 2.2.0 is a compatibility breaking update. Don't forget that the versioning of npm projects should respect : http://semver.org/ (i.e. not break compatibility when the first number does not change).

Most people having "node-cache": "^2.1.1" may encounter this kind of problem which is not what you want.

For now I just fixed my problem using "node-cache": "2.1.1" as depedency.

@matthieusieben
Copy link
Author

Also, note that cloning cached items may not be what users want. You should expose this as an option.

@smrchy
Copy link
Contributor

smrchy commented May 29, 2015

The cloning of items should be the default. Version 2.x should work as before with the default being not to clone. Version 3.x should have cloning disabled by default with an option to disable it.

@mpneuried
Copy link
Contributor

i unpublished the version 2.2.0. So it's not possible to install it any more.
Currently i'm testing it as version 3.0.0 with a new option useClones:false.

@mpneuried
Copy link
Contributor

Testing done. The version 3.0.0is online.

@matthieusieben
Copy link
Author

Does 3.0.0 with useClones: true works with promises ? I guess the expected behavior would be to create a new Promise that contains a cloned version of the fulfilled value/rejected reason.

@mpneuried
Copy link
Contributor

i don't think this will work.
If you want to cache promises use useClones: false as i described within the options docs

I'm using this module node-clone to clone variables, because i won't implement such a basic function again :-).
Of course you could open an issue in node-clone for a correct clone of a promise.

@matthieusieben
Copy link
Author

Yep, that would make more sense.
pvorb/clone#50

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

No branches or pull requests

3 participants