Optional timeout support for queries

[dustinlarimer]

Continuing conversation from keenlabs/KeenClient-Node#29.

/cc @alexkwolfe

[dustinlarimer]

@alexkwolfe there are a few overlaps with other issues/convos that point toward an interesting idea.

First: disclaimer: this is a loose sketch of an idea and will probably expose more of my ignorance about the things I'll be describing than anything else :)

Here's the source for client.run(): https://github.com/keen/keen-js/blob/node-client/src/core/lib/run.js This method just creates and returns a Keen.Request, which handles responses/errors appropriately and exposes its own .refresh() method for rerunning queries.

You can just as easily use this object directly:

var req = new Keen.Request(client, [query1, query2], callback);

In #168 @Jenius expressed interest in returning promises. This seems like a place where that would make a lot of sense.

Perhaps all of these ideas are pointing toward a light redesign of Keen.Request?

var req = new Keen.Request()
  .client(myClient)
  .timeout(1000)
  .fetch([query1, query2])
  .then(function(res){
    // do something with response
  })
  .catch(function(err){
    // handle err
  })
  .done();

..and since this object is returned by client.run(), one could optionally use then/catch methods rather than a callback or event listener:

var req = client.run([query1, query2])
  .then(function(res){
    // do something with response
  })
  .catch(function(err){
    // handle err
  })
  .done();

How does any of this sound?

[jescalan]

👍 from me, promises would make this much more useful in my mind

[alexkwolfe]

I don't use promises at all, but rather the standard Node.js callback convention. I could work around promises, if need be.

Also, I can't think of a reason why I'd need the Keen library to coordinate multiple simultaneous calls for me. Many times, I'm making concurrent calls to our database and Keen.IO, and use the async library to process them in parallel and callback when they've all completed.

[jescalan]

@alexkwolfe fwiw, most promise libraries will get you the same parallel functionality, in a slightly smoother way. Here's a post that thoroughly breaks down the two approaches and talks about migrating from the async library to promises.

[alexkwolfe]

All I'm looking for is the ability to set a timeout on a query.

Since the decision to use promises over callbacks is a matter of personal preference, to me there's really no point in worrying about which this library will use. I prefer callbacks. If @dustinlarimer feels that he wants to use promises, then I'll work around it. :)

[dustinlarimer]

@alexkwolfe i hear ya, I think we should hold off on promises for now. Query timeout control needs to happen, and I think Keen.Request is the place to do it.

I'm also in favor of moving toward the kind of composable interface described above (minus then/catch methods), and away from the big gnarly constructor model we have today. client.run() is a nice shorthand wrapper that covers most use cases, just like client.draw() for visualization, but there's way more functionality and control available when you use it directly.

[alexkwolfe]

Agreed. I think Keen.Request is a good spot for it.

[dustinlarimer]

@alexkwolfe sorry for the lag on this, afraid reworking Keen.Request will take a little more forethought. Any objections to putting this config on the client instead? (would apply to all queries)

[alexkwolfe]

One downside I can think of is that we'd have to instantiate a new Client if we wanted to use different timeouts for different queries. We are currently using different timeouts (I have in production a patched version of the other library that lets us use timeouts per request).

I'm not convinced setting the timeout on the Client is necessarily a big problem. Again, it seems to be something we could work around.

[dustinlarimer]

@alexkwolfe Managing multiple client instances as a workaround just sounds gross :) so I went ahead and reworked the Keen.Request object to handle this properly: 0ebcc63. Tests are passing, so I think this is just about ready to go.

[alexkwolfe]

Looks good to me. Thanks for adding the timeout. :)

[dustinlarimer]

@alexkwolfe https://www.npmjs.com/package/keen-js :)