Adds async option for find method - #19

[akofman]

I've just implemented the proposal of @basco-johnkevin because I have the same need :)
Please let me know if it's fine for you.

[akofman]

Ok I fixed this option if not specified from the schema.
I wonder if it would be better to also add the id of a doc as a parameter from the options in order to do :
db.find('authors', {id: 1, async:true}) and not db.find('authors', {startkey:1, endkey:1, async:true})

thoughts ?

[nolanlawson]

I think it would be better to just do !async here instead of async === undefined, so that it captures all truthy/falsy cases.

[nolanlawson]

Actually wait, I think maybe the best would be typeof idOrIds === 'undefined'. You're trying to determine whether idOrIds is undefined, right? We already know that async is false at this point in the code.

[nolanlawson]

Also the reason it's better to do typeof foo === 'undefined' rather than foo === undefined is because technically people can re-assign the undefined object in JavaScript

[akofman]

Yep you're right ! Thanks for your review. I'm gonna improve that in the evening. Good to know you're ok with this feature anyway.

[nolanlawson]

I don't think the does sideload if async option is force to false test is quite right. It seems to rely on the async === undefined checked described above, but the reason that res.books is undefined is not that docIds was unspecified – rather it's because the relationship was defined to be async in the schema. So the test would fail if you passed in e.g. 19 as the id, even though it should return the same result.

I think this PR needs a bit of work to ensure that the correct behavior occurs for the four possible cases (async true/false for schema/query), but it's going in the right direction. Thanks!

[akofman]

Hmmm not sure if I understand well your last comment ... I tried to modify the does sideload if async option is force to false test with return db.rel.find('author', {startkey:19, async: false}); and it seems to work well, is that what you mean ?

By default async equals false so indeed I defined it to true in the schema. Then if I don't find with the forced async option to false, I get res.books undefined as expected.

I updated the check with typeof.

[nolanlawson]

Sorry, what I meant to say is that I was unsure we had enough tests, and that I didn't really understand what the if (relationOptions && relationOptions.async && typeof idOrIds === 'undefined') line was doing exactly. (TBH I haven't looked at this code in a long time and I didn't write the async stuff.)

It kinda looks like the if condition is just checking for exactly the condition in the test and doesn't have anything to do with async vs non-async but I admit I should probably spend more time actually trying to understand this code.

[nolanlawson]

The !idOrIds test is also not quite right because it will fail if the id is 0 no? Based on an earlier part of the code, it looks like the true "there is no ID" test is: typeof idOrIds === 'undefined' || idOrIds === null.

[nolanlawson]

^ It's not clear to me if this means that is overrides in both cases. I.e. if the default is true and you set false here, it overrides, is that right? And if the default is false but you set true here, then it also overrides? If that's the case, then the code should probably need to check for typeof relationOptions.async !== 'boolean' as well as simple truthy/falsy, since it will be falsy in the case where it's not defined at all.

[nolanlawson]

I'm starting to understand the general idea behind the code, but I think if we had a bit more tests for all edge cases (default is true/false, override is true/false, override is not specified, idOrIds is specified or not, idOrIds is 0) then probably it would be clearer why I think the if checks are not quite right here. Let me know if you think I'm being off-base, but I'm just trying to make sure I give this PR a good thorough review. :)

[akofman]

Ok ! I'll back on this next week, sorry I had some stuffs to finish before ;)

[pocketarc]

Just bumped into this problem, which slows things down quite significantly in some queries where you don't need all that data to be pulled in (easy to bypass by going back down to allDocs, but not very pretty). Has there been any progress on this? Happy to help move it forward if this PR is stalled and a contributor is needed!