Skip to content

bucket.getFiles should document (at the top) the on() way of iterating. #673

Description

@jgeewax

Right now, in http://googlecloudplatform.github.io/gcloud-node/#/docs/v0.15.0/storage/bucket?method=getFiles

bucket.getFiles(function(err, files, nextQuery, apiResponse) {
  if (nextQuery) {
    // nextQuery will be non-null if there are more results.
    bucket.getFiles(nextQuery, function(err, files, nextQ, apiResponse) {});
  }
});

which is ... absurdly confusing -- I just want to iterate through all the files.

Can we change the examples to document the way to iterate through all files, and abort the traversal early if necessary?

bucket.getFiles().on('data', function(file) {
  console.log(file);
  if (some condition) {
    this.end();
  }
});

Activity

  1. jgeewax commented on Jun 17, 2015

    @jgeewax
    ContributorAuthor

    /cc @stephenplusplus : Kicking to you. I'm watching someone struggle with this right now.... and the recursive traversal stuff is confusing the hell out of them.

  2. stephenplusplus commented on Jun 17, 2015

    @stephenplusplus
    Contributor

    Separate and name the function to make it easier. We can document how to use streams, but it really isn't the solution. Streams are best when you indend to consume and pass-through all of the data to a connecting stream. It would be best to have a "recurse for me, then give me all of the results in my callback that only gets called once" option and avoid the concept of terminating early. The user always has pageSize and maxResults to avoid getting ridiculous size result sets.

  3. stephenplusplus commented on Jun 17, 2015

    @stephenplusplus
    Contributor

    I'll put a better proposal together tomorrow to demo what I'm thinking.

  4. jgeewax commented on Jun 17, 2015

    @jgeewax
    ContributorAuthor

    OK -- just the whole recursive calling next query... is confusing. People want to say "call this method again on the next page"...

  5. stephenplusplus commented on Jun 18, 2015

    @stephenplusplus
    Contributor

    just the whole recursive calling next query... is confusing.

    Yeah, it is. But it's the most unsurprising implementation. I don't think it would be less confusing to tell the user "your callback is going to be called an unknown amount of times, and each time the result set will be of a different length." I would rather use the callback once after we have all of the results consumed, and not even let the user know the nextQuery/API-determined pagination exists.

    As far as the streams go, I added support for the premature ending of a stream as part of the search PR: stephenplusplus@abca6f1 -- after that lands, I'll start to use stream-router throughout the rest of the library.

    The only thing about using streams to solve this problem is, it's kind of a weird use of them. The .on('data') API is nicer to deal with, but it's not typically where you see streams being used. A better use case for streams could be something like getting a bunch of data, filtering it down to a set you care about, parsing the metadata out of them and displaying the results in a terminal (bucket.getFiles().pipe(filterFilesOverTenMb).pipe(parseMetadata).pipe(process.stdout)). Streams usually aren't about getting a large set of results, then closing the pipe once you've found the result you're looking for.

    Still though, supporting pre-mature closing is probably a safe plan, since the user has ultimate say over how they want to do things.

    I still haven't thought of a great API to improve how nextQuery paginating works currently (other than removing it completely), but it's on my mind 😵

  6. jgeewax commented on Jun 18, 2015

    @jgeewax
    ContributorAuthor

    I don't think it would be less confusing to tell the user "your callback is going to be called an unknown amount of times, and each time the result set will be of a different length."

    But it's ... already being called an unknown number of times with the recursive way, right?


    I would rather use the callback once after we have all of the results consumed, and not event let the user know the nextQuery/API-determined pagination exists.

    Hmm... I really appreciated the idea of being able to say "when I've gotten all the items in this chunk, go go again"... this.nextPage() would have solved that problem....

    To put it in comparison to Python, we have an Iterator that basically lets you do:

    for item in bucket.list_objects():
      print item
      if item.name == 'whatever':
        break

    Which is paginating stuff behind the scenes. This is (I think) the .on('data', ...) syntax which... I'm OK with .

  7. stephenplusplus commented on Jun 18, 2015

    @stephenplusplus
    Contributor

    But it's ... already being called an unknown number of times with the recursive way, right?

    It depends what we're talking about. Going off of "People want to say "call this method again on the next page"...", I thought you meant you wanted getFiles to recurse itself, and execute that callback with each result set returned from the API.

    As far as "nextQuery()" vs "nextQuery" being an object, I fought my case in that other issue. I think it's better to give an object to the user, which I think is where we landed. That, plus documenting how to use the streams to early-exit. There will be a big sweep of all the dual methods soon after Search lands. I have to go through and implement the new stream-router, which will be a great time to catch the docs up.

    This is (I think) the .on('data', ...) syntax which... I'm OK with .

    Yeah, it will work this way.

  8. jgeewax commented on Jun 18, 2015

    @jgeewax
    ContributorAuthor

    I thought you meant you wanted getFiles to recurse itself

    I think I did mean that, but if it's not acceptable... then being able to say that "we'll run this function for each page until you tell us to stop (or we've gone through all the items)" is fine too...

    Bottom line though is... the whole nextQuery thing, and having to re-establish the callback again, or define it elsewhere and pass it in that calls the next query with a reference to itself and all that stuff is... not the way we should be telling users "here's how you page through all the data". It's hugely confusing...

  9. stephenplusplus commented on Jun 18, 2015

    @stephenplusplus
    Contributor

    I'd like to find a higher level solution for the nextQuery process as well. We can change nextQuery the object into a continueProcessing() function. Now I remember (I think) that is where we left off in our last discussion.

  10. jgeewax commented on Jun 18, 2015

    @jgeewax
    ContributorAuthor

    Or something like nextQuery.runAgain(); or something of that nature...?

  11. stephenplusplus commented on Jun 18, 2015

    @stephenplusplus
    Contributor

    Something like that might work. I'll be thinking on it 💭

  12. 18 remaining items

  13. added a commit that references this issue on Feb 2, 2026
  14. added a commit that references this issue on Feb 3, 2026
  15. added a commit that references this issue on Feb 17, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Labels

api: storageIssues related to the Cloud Storage API.

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions