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

JavaScript driver - Clean r.connect #1577

Closed
neumino opened this issue Oct 27, 2013 · 5 comments
Closed

JavaScript driver - Clean r.connect #1577

neumino opened this issue Oct 27, 2013 · 5 comments
Assignees
Milestone

Comments

@neumino
Copy link
Member

neumino commented Oct 27, 2013

r.connect in the JavaScript currently has two syntaxes:

r.connect( "localhost", function(error, connection) { ... } )
r.connect( { host: "localhost", authKey: "...", .. }, function(error, connection) { ... } )

If no one has strong objection, I would like to remove the first one. That way we would have the same syntax across the three drivers and that would make the driver a little less complex.
Note that you cannot pass a string like "localhost:28015" or anything else. Nothing is parsed.

The check on line 403 is not safe too (for the type of host)

If host is null, no error is thrown there, but one is thrown because we try to read the property host of null (line 72)

> r.connect(null, function(e, c) { } )
TypeError: Cannot read property 'host' of null
    at TcpConnection.Connection (/usr/lib/node_modules/rethinkdb/net.js:107:21)
    at new TcpConnection (/usr/lib/node_modules/rethinkdb/net.js:379:41)

If host is an array, no error are thrown, and the result is like passing an empty object.

If host is undefined, an error will be thrown on line 403, which make the test on line 67 useless.

@coffeemug
Copy link
Contributor

I would prefer to keep the first version. It's fairly common to run rethink on the default port, and the first version is a nice shortcut. I use it quite often, and it would be a pain to type out the full object.

We of course should fix the safety bug you mentioned.

@ghost ghost assigned neumino Oct 28, 2013
@neumino
Copy link
Member Author

neumino commented Oct 28, 2013

That makes sense.
I didn't check the ruby driver's code when I opened the issue (just the docs) and it looks like the ruby driver implements the same behavior.

Ok, let's just fix the safety check.

@neumino
Copy link
Member Author

neumino commented Oct 28, 2013

Branch michel_1577_connect
Review 996 assigned to @AtnNn

@jdoliner
Copy link
Contributor

Python also supports the first syntax btw. So we support it everywhere.

@neumino
Copy link
Member Author

neumino commented Oct 31, 2013

Merged in next as 97113cf
Cherry picked in v1.10.x

@neumino neumino closed this as completed Oct 31, 2013
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