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

page size 100 #9

Closed
wants to merge 3 commits into from
Closed

page size 100 #9

wants to merge 3 commits into from

Conversation

zicjin
Copy link
Contributor

@zicjin zicjin commented Feb 29, 2020

No description provided.

@zicjin zicjin closed this Feb 29, 2020
@zicjin zicjin reopened this Feb 29, 2020
@zicjin
Copy link
Contributor Author

zicjin commented Feb 29, 2020

1 All 6 ACCESS_TOKEN are invalid and used in the wrong way? document

2

// star-history.service.ts
// count step (to get indexes)
const step = NUMBER_OF_SAMPLES / samplesForPage;

It's very confusing. It took me a lot of time to understand the original meaning of this code. It just so happens that the default pagesize is 30, so you use the NUMBER_OF_SAMPLES constant?
In addition, I learned from here that pagesize can be changed, if changed to a larger value, does it mean that ACCESS_TOKEN usage can be reduced?

3 I got this compilation error of grpc in the windows. Unless you downgrade to node v10 or update firebase to the latest version. So I updated all the packages to the latest version using npm-check by the way.

Copy link
Owner

@pnowy pnowy left a comment

Choose a reason for hiding this comment

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

Can we split this dependencies upgrade? I mean as a separate pull request?

@pnowy
Copy link
Owner

pnowy commented Feb 29, 2020

Ad.1 Yes - the Github deprecated the approach (https://developer.github.com/changes/2019-11-05-deprecated-passwords-and-authorizations-api/#authenticating-using-query-parameters) but I think these tokens should be removed at all (those tokens are not active anymore).

I think the right approach would be to add some login screen to user Github auth in order to retrieve the token (without scopes - they are not needed in this case) or some additional modal to provide access token in order to increase rate limit - otherwise there will be the problem.

Ad.2 The constant NUMBER_OF_SAMPLES was the number of samples for chart (for popular repository there could be a lot of pages/stars so it was enough to get only 30 samples from different pages to get correct chart). But yeah - I can agree that this code can be refactored because is quite confusing and not easy to read.

I would split this changes int smaller separte changes.

Ad. 3. Lets upgrade it as separate PR. Will you do that or should I?

@zicjin zicjin closed this Mar 1, 2020
@zicjin zicjin reopened this Mar 1, 2020
@zicjin zicjin closed this Mar 1, 2020
@zicjin
Copy link
Contributor Author

zicjin commented Mar 1, 2020

I built a new pull request.

In fact, I prefer the way auth token works in this project. There are many star-history projects that are authorized by the user OAuth elsewhere. But I think the current way of using Auth token is advantageous and convenient enough for the product. OAuth is always a threshold. If we can collect 20 token, we can have 100,000 requests/per hour , it's enough. And over time, the cache hit ratio of popular repo will become higher and higher.

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