Remove some old stuff that is not needed anymore #809
Conversation
| @@ -211,14 +210,14 @@ RedisClient.prototype.do_auth = function () { | |||
| self.send_anyway = true; | |||
| self.send_command("auth", [this.auth_pass], function (err, res) { | |||
| if (err) { | |||
| if (err.toString().match("LOADING")) { | |||
| if (/LOADING/.test(err.toString())) { | |||
There was a problem hiding this comment.
performance nit: creating these regular expressions as vars and moving them out of the send_command code-path would be a slight improvement.
There was a problem hiding this comment.
however "auth" is pretty rare so... 😛
There was a problem hiding this comment.
two thoughts here: can we not use err.message rather than err.toString()?
also, wouldn't all of these be better off using indexOf? err.message.indexOf('string') !== -1
There was a problem hiding this comment.
great points @simontabor
There was a problem hiding this comment.
@simontabor err.message is very likely better but as far as I know a precompiled regex with .test is faster than .indexOf and it's at least more readable in my opinion. It's difficult to write a proper test to compare the regex against the .indexOf as v8 is very likely inlining everything right away.
There was a problem hiding this comment.
I wouldn't worry too much about perf here because it's in the auth stanza, which is only used by a subset of Redis users, and even then is only called once per client connection, but not only that it's in the error pathway of the auth stanza, so this is a super rare code path.
I shouldn't have brought perf up ealier 😁
Mostly just need to worry about correctness
|
Some notes above, this one seems to be a fairly significant PR |
|
Feedback incorporated |
|
LGTM, thanks for helping to clean up this library! |
Remove some old stuff that is not needed anymore and fix js parser sending non-Errors as errors.
No description provided.