Implement basic AvatarList.astro #17
Conversation
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/pikapkg/astro-docs-preview/32dRV2xb4RUojQufitZsMCGxxWj6 |
|
I still need to make it detect rate-limiting from Github and such.. It probably makes sense to use https://www.npmjs.com/package/octokit#user-content-octokit-api-client, but I had problems running it, snowpack was complaining about not installing esbuild.. |
|
I'm getting a 403 Forbidden through my personal token, but I can load the URL fine in Firefox... so A. It's probably a good idea for me to figure out how to use Octokit and B. auth is confusing |
|
Design suggested by @mrbrianhinton:
I artificially increased the number of contributors for this screenshot by not deduplicating the commits, but it shows only the last three contributors as avatars + the rest are just a number! |
jasikpark
changed the title
jasikpark
changed the title
src/components/AvatarList.astro
Outdated
| export let path; | ||
| // fetch all commits for just this page's path | ||
| let url = `https://api.github.com/repos/snowpackjs/astro-docs/commits?path=${path}`; | ||
| let token = import.meta.env.SNOWPACK_PUBLIC_GITHUB_TOKEN ?? throw Error('You might not have "SNOWPACK_PUBLIC_GITHUB_TOKEN added for escaping rate-limiting.'); |
There was a problem hiding this comment.
can we leave this as optional? It would be a big point of friction to need to generate an access token just to start up dev.
If it's a concern that this throws an error when rate limited: we can catch that error, ignore it, and then return an empty avatar list. This would only happen in dev, but then still let you provide your own token if you need to
There was a problem hiding this comment.
That certainly sounds reasonable
src/components/AvatarList.astro
Outdated
| Authorization: auth, | ||
| "User-Agent": "astro-docs/1.0" | ||
| }, | ||
| }).then((res) => { |
There was a problem hiding this comment.
astro supports top-level await! It would be nice to see this instead:
const res = await fetch(...
const data = await res.json();
There was a problem hiding this comment.
I can certainly do that, I just like scoped vars is all!
There was a problem hiding this comment.
I know, I always hate that await-fetch requires that unnecessary res var when all I want is the JSON
|
LGTM with a few comments! just added the secret, redeploying now |
Currently the images are hotlinked, so it might be the case where it makes more sense to pull in the images as assets programmatically and then reference them locally, but that's for once Astro has images figured out a bit more. I fixed the editHref to actually point to this repo There needs to be a Github Personal Access Token with `public_repo` permissions set to `SNOWPACK_PUBLIC_GITHUB_TOKEN` to remove Github rate limiting.
…oesn't seem to support throws from what I can tell though?
…catch and print to stderr
…owpack warnings prevents viewing it, so it's just console.log
|
Something fun is that since we're sending all snowpack warnings into the void, I can't see the output of |
|
I think that was removed in 0.14.0, we shouldn't be swallowing anything anymore. err... looks like we are doing some |
|
Success! I had to add the secret env variable to Vercel, not Github |
|
|
|
Could someone review this? It should be good to go for merging. |
Currently the images are hotlinked, so it might be the case where it makes more sense to pull in the images as assets programmatically and then reference them locally, but that's for once Astro has images figured out a bit more.
I fixed the editHref to actually point to this repo
There needs to be a Github Personal Access Token with
public_repopermissions set toSNOWPACK_PUBLIC_GITHUB_TOKENto remove Github rate limiting.