datastore: support streams with request.get

[stephenplusplus]

RE: #750 (comment)

This adds streamRouter to the get method from Datastore, as talked about in #750.

Here's a problem, though. streamRouter was intended for handling multiple results, like the results of a query where the user doesn't know how many to expect back. Always giving the user an array of the results to their callback makes sense there, but with dataset.get, the user knows exactly how many responses they will get back. So, that makes this awkward:

var key = dataset.key(['Company', 123]);

// Before:
dataset.get(key, function(err, entity, apiResponse) {
  // entity = the entity obtained from the key
});

// After:
dataset.get(key, function(err, entities, apiResponse) {
  // entities = array of results (will have 1 result from the 1 key)
  // entities[0] = the entity obtained from the key
});

On the bright side, the user can now get streaming results from this method:

dataset.key(keys).on('data', function(entity) {});

I haven't thought of a great solution to the problem above yet. I'm leaning towards saying streamRouter isn't the right solution in its current form. Ideas welcome :)

[callmehiphop]

// begin crazy talk

I agree that streamRouter in its current state probably isn't the way to go. At first I was going to say that we should just make a separate method for streams since this api is a bit different than our typically streamRouter use case -- however, I think a similar problem may already exist (well, whether or not it's actually a problem is probably up for debate)

Typically there's an option to set the maximum number of results to return, so what's the expectation when that max is set to 1 (not sure how likely this is to happen). As it is today, even though we only request 1 result, we still return an array -- this of course makes total sense, but it might offer slight inconvenience if the user is only expecting 1 result.

What if we were to add logic that checked if the max was 1 and if so, returned the singular item instead of an array. After which maybe it would be possible to somehow set the max to 1 if we saw that this particular method only received a single key?

¯_(ツ)_/¯

[rekow]

This is super awesome to see!! Still want an issue for tracking or is the comment ref enough?

This might be naive, but couldn't you continue to unpack the single-item array much like in the current implementation of get in the case where a single key is passed? Fetching a single item seems mutually exclusive with pagination, so there shouldn't be any case where streamRouter runs the request as a stream if it's passed a callback - in that world there's no reason entities needs to be an array in the makeReq_ callback, right?

Just seems a bit easier than handling the unpack at the streamRouter level - then you'd have to update other streamed methods to potentially expect a single value as well, I think.

[stephenplusplus]

@davidrekow no worries about the issue :)

Typically there's an option to set the maximum number of results to return, so what's the expectation when that max is set to 1 (not sure how likely this is to happen). As it is today, even though we only request 1 result, we still return an array -- this of course makes total sense, but it might offer slight inconvenience if the user is only expecting 1 result.

I think the road of least surprises is where we want to go. If we can document our method as always returning an array, nobody would expect to get an unboxed array under any conditions. Your suggestion is a pretty creative way to solve the problem, though, and it would work if we wanted our other APIs that accept maxResults to unbox the results for the callback. I think that would create unexpected results, however.

I'm leaning towards just handling the stream logic in this method individually, and avoiding using streamRouter altogether. Maybe as we go on with our library, we'll find other similar places and we can start to find ways to abstract the logic.

[stephenplusplus]

Let me know how :( my latest commit is.

[callmehiphop]

I think for the sake of API consistency, it's undoubtedly a step in the right direction. What do you think the likeliness of this issue popping up again is? I only ask because I could see it resurfacing elsewhere, so maybe we'd want to put this logic into its own module

[stephenplusplus]

I only ask because I could see it resurfacing elsewhere, so maybe we'd want to put this logic into its own module

It definitely might. I can't think of any off hand like this that aren't already using streamRouter, though. If the time comes when we can re-use this logic, we can module out?

I'll start on tidying up this PR.

[stephenplusplus]

PTAL :)

[callmehiphop]

Looks good, thanks!