Skip to content
This repository was archived by the owner on Feb 18, 2021. It is now read-only.

Conversation

dfellis
Copy link

@dfellis dfellis commented Mar 22, 2014

index.js Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

this.emit('error', err)

Copy link
Author

Choose a reason for hiding this comment

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

Turn this into an EventEmitter for a single event?

Copy link
Contributor

Choose a reason for hiding this comment

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

you also need to re emit errors from redisClient.

@twolfson
Copy link

One of these finally. I would always point to EventVat and be sad it never had a callback system for easy swapping =(

https://github.com/hij1nx/EventVat

@dfellis
Copy link
Author

dfellis commented Mar 22, 2014

@twolfson This library needs to be API-compatible with this one. The API is "inspired" by ES6 Maps, but deviating in that it's async (with *Local methods if you want speed but don't care about cache correctness).

In this case, redis is being used as a shared cache between many workers across servers, so the original temp directory file cache won't do.

index.js Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

want to remove this print

Copy link
Author

Choose a reason for hiding this comment

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

yes

Copy link

Choose a reason for hiding this comment

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

catch exception / callback error?

@Raynos
Copy link
Contributor

Raynos commented Mar 25, 2014

Other then the print and the callback(null, bool) this seems good.

index.js Outdated
Copy link

Choose a reason for hiding this comment

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

Should we emit a ready event after all keys are loaded in the eager case, and right away in the lazy case?

Copy link
Author

Choose a reason for hiding this comment

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

Sure.

@Raynos
Copy link
Contributor

Raynos commented Mar 25, 2014

This looks good to me now.

Raynos added a commit that referenced this pull request Mar 25, 2014
@Raynos Raynos merged commit a4e2360 into master Mar 25, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants