Skip to content

Convert snake_case options to camelCase before sending into tls || net (FIXES #1103) #1104

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

Closed
wants to merge 2 commits into from
Closed

Conversation

ghost
Copy link

@ghost ghost commented Jul 14, 2016

Convert snake_case options to camelCase before sending into tls || net (FIXES #1103)

var key;
while (key = keys.pop()) {
opts[key.replace(/_([a-z])/g, function (g) { return g[1].toUpperCase(); })] = this.connection_options[key];
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This is going to run on every reconnect attempt. Instead, the fix could be placed where the options are converted to camel_case. That would only run once for each client. The clone function could receive a extra argument to skip the converting tls options to camel_case and special handle that by checking if the key name is tls.

Copy link
Author

Choose a reason for hiding this comment

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

I don't understand the sacred meaning of conversion to snake_case so I wrote my code right before sending connection options into tls or net, the same way it was converted. Sorry, this code is not in my style. May be this quest is for the person who has implemented the conversion. Do it as you see fit please.

Copy link
Contributor

Choose a reason for hiding this comment

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

The reason for the conversion is that node_redis was written in snake_case and camelCase support was added by converting these values to snake_case and therefore both is supported.

Copy link
Author

Choose a reason for hiding this comment

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

In my opinion the solution is to leave connection_options as is, I mean do not clone it.
Instead of:

function RedisClient (options, stream) {
    // Copy the options so they are not mutated
    options = utils.clone(options);
    EventEmitter.call(this);
    var cnx_options = {};
    ...
}

Something like:

function RedisClient (options, stream) {    
    EventEmitter.call(this);
    var cnx_options = {};
    ...
    // Copy the options so they are not mutated
    options = utils.clone(options);
    ...
}

Copy link
Contributor

Choose a reason for hiding this comment

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

The options are already cloned before they are passed to the constructor. This is done in the createClient function.

BridgeAR pushed a commit that referenced this pull request Oct 31, 2016
BridgeAR pushed a commit that referenced this pull request Oct 31, 2016
@BridgeAR
Copy link
Contributor

@DaB00 thanks a lot for your contribution! And sorry it took so long...

moomou pushed a commit to smyte-forks/node_redis that referenced this pull request Dec 20, 2019
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

Successfully merging this pull request may close these issues.

2 participants