JS driver - Change the syntax of run

[neumino]

run uses a really strange syntax when the user wants to pass some options.

The two syntaxes are

query.run(connection, callback)
query.run({connection: connection, useOutdated: true}, callback)

The second syntax should be

query.run(connection, options, callback);

See second example in http://docs.nodejitsu.com/articles/javascript-conventions/how-to-create-default-parameters-for-functions

[coffeemug]

@neumino -- would you mind finding one or two other examples of how they do it in node land? If we make a change, we should make sure it's widely accepted.

[neumino]

I think it's safe to do the change.
The module fs (native module) uses this syntax for readfile

fs.readFile(filename, [options], callback)

See http://nodejs.org/api/fs.html#fs_fs_readfile_filename_options_callback

[coffeemug]

Ok, sold.

[mlucy]

Since this would break most existing code, maybe it should be a post-LTS thing?

[baruch]

I actually believe you'll be better off breaking things now rather than later. People will get used to things the way they are and you will have more users to upset later on than now.

[AtnNn]

run can be made to accept both calling conventions in a backwards compatible way. There is no need to break any code.

[coffeemug]

I'd actually prefer to break code here. Having two distinct ways to call a function is pragmatic but has a bad smell to me. I think we should move to what the rest of the world is doing, and then tell people to change the code. It's a simple enough change that it's worth to just do it IMO.

[mlucy]

We should at least add special error logic to detect when people are using the old syntax and tell them that we changed it.

[coffeemug]

We should at least add special error logic to detect when people are using the old syntax and tell them that we changed it.

👍

[coffeemug]

@mlucy -- also, you're really a fan of the "did you mean?" approach, aren't you :-D

[neumino]

Ok, I can update the driver and make it backward compatible.

In case people use the old syntax, I'll just print a warning. I saw a few libraries doing it.

[coffeemug]

I'd prefer to error on old syntax and print a clear error message. I really don't like the idea of having two different ways to do something as simple as create a connection. Having one way is just better.

[neumino]

Up in review 1243 assigned to @AtnNn
Branch michel_1890_run

[neumino]

The behavior is to log a message in case the user uses the old interface.

The new syntax being run(connection[, options], callback)

[coffeemug]

What message gets printed? Where does it get logged?

[neumino]

The current is:
This syntax is depreciated. Please use run(connection[, options], callback).

And it's logged with console.log (util is not available on the browser).

[AtnNn]

I think it is better to not show any warning and to mention that the old behaviour is deprecated in the documentation and in the release announcement.

If you really want to show it warning, ideally:

• It should go to stderr
• It should be possible to turn it off (perhaps by doing r.disable_warnings = true)
• it should only appear on the first call to run that is deprecated

[neumino]

That's ok for me. I just removed the warning.

[coffeemug]

Err, I don't think silently supporting an old, misguided function signature is a good idea. At the very least we should change the signature in the docs, and have the old signature in a deprecated example.

I think we have different people with different philosophies on compatibility, and it's hurting us because there is no coherent approach throughout the whole product. We need to make a decision on #1960 and stick to it everywhere. I don't think we should do this before the release, but it's really important for all of us to commit to a single approach. Let's talk about it together once 1.12 is out.

[coffeemug]

(To clarify, I don't think we should delay shipping this issue, just that we should commit to a compatibility approach after 1.12 and follow it strictly after that)

[neumino]

On second though, we should keep the message logged because if a previous user write new code, he must be aware that he's using a depreciated syntax.

• I'll log it in stderr.
• It will appear just once

We shouldn't let people disable it. If they have to update their code with r.disable_warnings = true, they should just write run(connection, options, callback).

[mlucy]

I agree with @coffeemug that we should ship this and argue after, but I think we shouldn't support a deprecated syntax here. We should throw a descriptive error describing the new syntax instead.

[neumino]

The implemented behavior now is to print the message on stderr, and print it only once.

I talked a little with @AtnNn and I understand better his worries. We should definitively talk about #1960 when 1.12 is out.

[neumino]

Note that util.print (shipped with node) log the deprecation too.

> util.print("hello")
util.print: Use console.log instead
hello

[coffeemug]

The implemented behavior now is to print the message on stderr, and print it only once.

What do you mean by "print it only once"? Is it once per process invocation? How does that work?

[neumino]

If you run two queries using the old syntax for run, the warning will be printed once.
I keep a flag on whether the warning has been printed before or not.

If you kill node and restart your script, the message will be printed again (but once per "session")

[coffeemug]

Ah, got it, thanks.

[neumino]

Merged in next as 45843c7
Ship 1.12

[jmdobry]

@neumino @coffeemug @AtnNn I understand the need for this change, but now I am confronted with the question of whether my library (reheat) can support 1.11.x and 1.12.x. Reheat currently depends on 1.11.0-4. Can I safely change to 1.12.0-0?

Should I try to support anything other than the current version of RethinkDB? What is the compatibility between new servers and old drivers, and old servers and new drivers? Is there a detailed changelog anywhere that lists breaking changes like the one in this issue?

In my testing of the run method I've gotten the following (using reheat's integration tests).

syntax	JS driver	RethinkDB	result
old	1.11.0-4	1.11.3	works
old	1.11.0-4	1.12.4-0	works
old	1.12.0-0	1.11.3	works, with deprecation warning
old	1.12.0-0	1.12.4-0	works, with deprecations warning
new	1.11.0-4	1.11.3	error thrown
new	1.11.0-4	1.12.4-0	error thrown
new	1.12.0-0	1.11.3	works
new	1.12.0-0	1.12.4-0	works

If it works, my ideal solution would be to have reheat depend on the 1.12.0-0 driver. The last two lines in the table above seem to indicate that I can upgrade safely.

[neumino]

Hello @jmdobry

First, we are sorry for all the inconvenience the changes we made can produce. We are also aware that this involves some work for people maintaining libraries like reheat. We really appreciate your work, and will try our best to stop making breaking changes as soon as possible.

To answer your questions, the changes for the syntax of run is backward compatible and it is safe to update the driver to 1.12 in your package.json file. The only thing that can happen is that the user will see a deprecation warning (it gets printed only once though).

If you run into a bug/crash, feel free to open a GitHub issue, and I'll make sure to fix it -- but as far as I have tested it, the 1.12 driver works with the 1.11 and 1.12 server (like your tests show too).
The internal message sent to open the connection did not change.

If you have more questions, let me know.

[jmdobry]

@neumino Thanks. I'll upgrade then.

[dulichan]

Hey guys,
I know this is an old issue but how do I get this to work with promises?

db.then(function(con) {
    return r.table('authors').run(con);
}).then(function(cursor) {
    return cursor.toArray();
})

Above fails with RethinkDB warning: This syntax is deprecated. Please userun(connection[, options], callback).Possibly unhandled RqlDriverError: Found deleted which is not a valid option. valid options are error.

Driver version is 1.15.0-0 and RethinkDB version is 1.15.0-1

[neumino]

This works fine for me with the driver 1.15.0-0, with db = r.connect()

My guess is that db is not a promise that returns a connection. How did you define it?

[dulichan]

Here is how I defined db

var db_promise = r.connect({
    host: 'localhost',
    port: 28015
});

[neumino]

Can you try to print con with console.log(con) just before calling run.
Do you by any chance leak db somewhere and overwrite it with the result of a write query?

[dulichan]

Result of log

{ _bitField: 0,
  _fulfillmentHandler0: undefined,
  _rejectionHandler0: undefined,
  _promise0: undefined,
  _receiver0: undefined,
  _settledValue: undefined,
  _boundTo: undefined }

You can check out https://gist.github.com/dulichan/542059c00a99b7a5a999 for the whole code.

[neumino]

See https://gist.github.com/dulichan/542059c00a99b7a5a999#comment-1314935

The function of the second then takes as argument the result of what is returned by the function in the first then (therefore an object like {inserted: X, deleted: 0, unchanged: 0, ...}

You can bind the connection to this like in the comment.
You can check https://promisesaplus.com/ and https://github.com/petkaantonov/bluebird/blob/master/API.md#promisebinddynamic-thisarg---promise for more information.

[dulichan]

Oh - I thought r.connect returns a promise back. Looks like I have to bind a new promise to the promise chain using bind. Any reason why this wasn't implemented in the usual pattern where the connection is returned for the next promise?

Thanks @neumino

[neumino]

@dulichan -- I think there's a misunderstanding. r.connect returns a promise.
And you have the following behavior:

var db = r.connect()

db.then(function(conn) {
    // conn is a connection
});

db.then(function(conn) {
    // conn is a connection again
    var p = new Promise(function(resolve, reject) { ... })
    return p
}).then(function(result) {
    // result is the argument that `p` is going to be resolved with
})

So you can call then on db multiple time to retrieve the same connection, but if you chain two then, the second then is going to be called with the result of the promise returned by the first then.

[dulichan]

Got it. I forgot that rule of the promise. Thanks @neumino