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

Add Query Functionality #26

Merged
merged 4 commits into from
May 25, 2018
Merged

Conversation

LaughDonor
Copy link
Collaborator

@LaughDonor LaughDonor commented May 23, 2018

To use:

var db = FirestoreApi.getFirestore(email, key, project);
var path = "string/to/collection", //Required.
    fieldname = "nameOfFieldToFilter",
    operator = "==", //Can be ==, ===, <, <=, >, >=, NaN, or null (the last 2 don't need a value)
    value = "valueToCheck",
    dir = "asc"; //Direction can be asc, ascending, desc, dec, descending. Defaults to ascending.
var results = db.query(path).where(fieldname, operator, value).orderBy(fieldname, dir).offset(0).limit(20).execute();

All chained methods between query() and execute() (not inclusive) are optional.
Multiple paths can be searched. i.e. db.query(path1, path2, path3, etc...)

The Wiki will also need to be updated if this gets merged in.

Copy link
Owner

@grahamearley grahamearley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two more notes in addition to the comments I left:

  1. If I set the path for a query to be a nested collection (like the "string/to/collection" given in your example), I get this error back in the response:Error: Collection id "query_test/dummy/nested" is invalid because it contains "/". I didn't look too much into the cause of it — does anything jump out to you?
  2. What do you think of having the getDocuments function route through the query function instead of getting a list of documents from a GET request for a collection? It looks like that doesn't require dealing with the page token, which would be nice. I'm in favor of this option, so if you agree, could you update that? Then we could finally close Add method for getting list of documents (as objects) from a collection #5!

Other than these notes, this is great. Definitely the most thorough PR this library has seen yet! 🙌

}

const query = {
from: from.map(function (collection) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line confused me for a while since I was thinking from was a string. It's an array, though, right? Could you update the class-level comment to say this parameter is an array?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from gets transformed into an array. I guess I overlooked that JSDoc comment. Thanks!

Util.js Outdated
@@ -12,6 +12,10 @@ function isInt_ (n) {
return n % 1 === 0
}

function IsNumeric_ (val) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason you capitalized this function's name?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy pasta oversight.

}

function getObjectFromResponse_ (response) {
return JSON.parse(response.getContentText())
function fetchObject_ (url, options) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love this!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it gets even better in the other PR. :)

@LaughDonor
Copy link
Collaborator Author

  1. Good catch on the nested collection. I didn't create a test case for that. Will check that out.

  2. I have mixed feelings about this. On one hand it would create less fetching overhead, and on the other, I really love the updated implementation and code from the other PR that simplified the logic.

@LaughDonor
Copy link
Collaborator Author

LaughDonor commented May 25, 2018

  1. As far as the nested collection is concerned, I'd like to solve it in the other PR for two reasons: a) Avoid implementing, and reimplementing a solution. b) The solution would be cleaner since I would like to group nested collections together. (i.e. "coll1/doc1/nest1" and "coll1/doc2/nest2" would be grouped together, and "coll2/docX/nest1" would be a separate request, which would then be batched together afterward).

  2. After some consideration, I think it would be optimal to do as you suggest, and route the getDocuments to use query_ internally.. also in the other PR for reason a) above.

@grahamearley
Copy link
Owner

@LaughDonor Sounds good. I haven't gotten a chance to look at the other PR yet, but I'm looking forward to it! Once this PR has the getDocuments method updated, I'll approve it.

I'm going to wait to pull this in, though, until that other PR has the fix (even though it's probably not a big deal to have code in master that has a slight bug, I'd like to be able to quickly merge in a PR with the solution afterward).

@LaughDonor
Copy link
Collaborator Author

The other PR will have that fix in for getDocuments. So you should be able to merge this in. That way the getDocuments functionality won't be affected (since it'll be dependent on the query function).

@grahamearley
Copy link
Owner

@LaughDonor Oh right, my bad! Missed that part.

Okay, in it goes. 🚀

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

Successfully merging this pull request may close these issues.

2 participants