Fix updated at time to use default branch commits and check monorepo directories #333
Conversation
- Uses graphql api which allows us to make a single request to fetch the package.json and other details - Fixes updated time so it only checks commits on the default branch - Get the correct time for monorepos as it checks for commits within the folder
| @@ -1,8 +1,7 @@ | |||
| { | |||
| "build": { | |||
| "env": { | |||
| "GITHUB_CLIENT_ID": "@github_client_id", | |||
| "GITHUB_CLIENT_SECRET": "@github_client_secret" | |||
| "GITHUB_TOKEN": "@github_token" | |||
There was a problem hiding this comment.
I don't think it's possible to use a client id and secret with the graphql api so this needs to be updated to a github token.
| // const repoRequestCache = {}; | ||
|
|
||
| export const fetchGithubData = async data => { | ||
| const getUpdatedUrl = async url => { |
There was a problem hiding this comment.
Sometimes the graphql API doesn't pick up redirects. This function gets the updated url using the rest API to retry.
| hasIssues: json.hasIssuesEnabled, | ||
| hasWiki: json.hasWikiEnabled, | ||
| hasPages: json.deployments.totalCount > 0, | ||
| hasDownloads: true, |
There was a problem hiding this comment.
I couldn't find this in graphql, and I'm not really sure what it is meant to represent. I've set it to true to maintain backwards compatibility as every repository listed in the data was true
There was a problem hiding this comment.
I think it probably refers to the repo having files/installers attached to releases, but not sure tbh.
| @@ -45,9 +45,9 @@ export type Library = { | |||
| license: { | |||
| key: string; | |||
| name: string; | |||
| spdx_id: string; | |||
| spdxId: string; | |||
There was a problem hiding this comment.
These two fields have been updated by graphql. I couldn't see them being used anywhere but it made more sense to just leave them in camelcase to match the rest of the output rather than having to modify them. I can revert this if it's going to cause a breaking change.
|
hey Alex, thank you so much for picking this back up 👍 @brentvatne I think this new approach is totally worth as the original problem highlighted in my issue seems to still stand. |
|
i pulled the branch locally and i had trouble fetching data using we should update aside from resource limit related things, this looks great :) thanks! |
|
merged in 4c1b864 - thanks a lot! if you can follow up with a fix for |
fixes #20
Why
The updated time was including any updates to the repo, such as issues created or commits in other branches. I think users would expect the updated time to either represent the last time a commit was made to the default branch e.g. master or the last publish time.
The updates would always be the same for monorepos, so every time large ones like expo had any updates the first few pages would be expo projects, even though only one of them had been updated.
Changes
Checklist
If you added a feature or fixed a bug:
Screenshot showing expo monorepo now has different updated dates that matches the folder location and master branch
Thanks to @kelset for providing initial graphql query in #50