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

[Feature Request] Ability to fetch a single page of items in a list #406

Open
jeanettehead opened this issue Dec 2, 2016 · 8 comments · May be fixed by #583
Open

[Feature Request] Ability to fetch a single page of items in a list #406

jeanettehead opened this issue Dec 2, 2016 · 8 comments · May be fixed by #583

Comments

@jeanettehead
Copy link
Contributor

Would be nice to be able to fetch only a page of repositories, repo commits, and other lists instead of fetching the entire thing every time. Currently they call into _requestAllPages which automatically fetches everything from github, which can take awhile for long lists.

@mtscout6 suggested on gitter that a generator function would be a good way to accomplish this.

If there's a good direction to go in, I can probably work on this.

@mtscout6
Copy link
Member

mtscout6 commented Dec 2, 2016

So this gets interesting because there's a few ways to do this. First is to be transparent about paging. So, something like:

for (let page of request()) {
  for (let item of page.items) { ... }
}

Can't say that I'm a huge fan of that approach. Though if you move to using the newer for await support from Babel as proposed with the async generator transform and outlined as a tc39 proposal

Then you could do something like this:

for await (let item of request()) {
  ...
}

Though the ES5 equivalent gets really messy:

function forEach(ai, fn) {
  return ai.next().then(function (r) {
    if (!r.done) {
      fn(r);
      return forEach(ai, fn);
    }
  });
}

var item = 0;
forEach(request(), function(val) { item += val.value })
  .then(function () {
    console.log(item);
  });

Essentially downstream users that use Babel would be the ones to win here, those not Using Babel would need to wait a few years for the Spec changes to replicate to the different JavaScript interpreters. If we did the async generator approach then we should callout in the docs that Babel is recommended and that we wouldn't support users not using Babel.

@clayreimann Thoughts?

@jeanettehead
Copy link
Contributor Author

I'm down for that. A (stupidly simple) alternative would be to allow the users to send a page option through and fetch just that page. As it stands, sending a page option causes an infinite loop of requests, which seems like a bug.

@jeanettehead
Copy link
Contributor Author

@mtscout6 I'd still like to make this happen. I'm down for either approach - do you have any thoughts?

@mtscout6
Copy link
Member

The page option is a valid option, and much simpler too. Go ahead and submit a PR for it.

@jeanettehead
Copy link
Contributor Author

Opened a pull request: #409

@jeanettehead
Copy link
Contributor Author

This would be all set, but when I npm install https://github.com/github-tools/github.git#3.1.0, it's unable to be imported into my project because it can't find the dist/components/GitHub.js specified as the main entry point in the package.json. Is this fixable or can a new release get published to npm?

@jeanettehead
Copy link
Contributor Author

@mtscout6 @clayreimann any update on my above question?

@mtscout6
Copy link
Member

That dist folder is generated during the CI. With the CI failing and not publishing to npm that's your problem. I have not had time to dig into the cause of the CI failure, and I do not have permissions to push directly to npm bypassing CI. I wish I could say that I had the time to look at it, but I don't for some time.

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 a pull request may close this issue.

2 participants