-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
refactor: migrate cluster-related code to TypeScript #717
Conversation
220d2ba
to
52982fb
Compare
52982fb
to
e8375ce
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some comments.
You might want to consider cleaning up JSDoc in general as you touch code btw.
lib/cluster/index.ts
Outdated
/** | ||
* Creates a Redis Cluster instance | ||
* | ||
* @constructor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can probably omit most of those @constructor
and other class related hints.
lib/cluster/index.ts
Outdated
import ConnectionPool from './ConnectionPool' | ||
import {NodeKey, IRedisOptions, normalizeNodeOptions} from './util' | ||
|
||
var Deque = require('denque') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of those requires can probably be moved to import (namely node and some of your own).
Everything should probably be const
lib/cluster/index.ts
Outdated
* | ||
* @private | ||
*/ | ||
_handleCloseEvent () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can get rid of @public
and @private
and use the keywords instead.
} | ||
return asCallback( | ||
Promise.all(this.nodes().map(function (node) { | ||
return node.quit() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this sometimes would throw if node that we issue quit to is already in the disconnected state (often in the tests where redis instance is down before the quit is processed)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! I'll fix this in a separate commit.
lib/cluster/index.ts
Outdated
private setStatus(status: ClusterStatus): void { | ||
debug('status: %s -> %s', this.status || '[empty]', status) | ||
this.status = status | ||
process.nextTick(this.emit.bind(this, status)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this.emit.bind could be saved as a reference, ie boundEmit
and then we can process.nextTick(this.boundEmit, status)
- that should be a bit faster
No description provided.