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

Add failsafes in case Octokit api fails. Statically-stored most information in .json files. Change revalidation period for projects page. One issue with importing several variables and functions, where I had to manually import each from their respective file, instead of importing all from '../util'. #117

Closed
wants to merge 9 commits into from

Conversation

JCamyre
Copy link

@JCamyre JCamyre commented May 26, 2022

Add failsafes in case Octokit api fails. Statically-stored most information in .json files. Change revalidation period for projects page. One issue with importing several variables and functions, where I had to manually import each from their respective file, instead of importing all from '../util'.

Fixes #114

…mation in .json files. Change revalidation period for projects page. One issue with importing several variables and functions, where I had to manually import each from their respective file, instead of importing all from '../util'.
…mation in .json files. Change revalidation period for projects page. One issue with importing several variables and functions, where I had to manually import each from their respective file, instead of importing all from '../util'.
@JCamyre JCamyre requested a review from matthewcn56 May 26, 2022 21:56
@JCamyre JCamyre added the enhancement New feature or request label May 26, 2022
…ts with the ucla-opensource tag. Add short blurb about the page and indicate that it is a work in progress.
@JCamyre JCamyre linked an issue May 27, 2022 that may be closed by this pull request
Copy link
Member

@matthewcn56 matthewcn56 left a comment

Choose a reason for hiding this comment

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

This is really outstanding work Joseph! There's just a small nitpicks I have here and there, but overall you truly caught all the possible exceptions well and it's pretty smart to write to the json everytime we revalidate!

I just left a couple nitpicks with regards to semantic HTML, package-lock.json conflicts, and some small abstraction with the revalidation constant, but nothing major.

This is great work, thanks for taking a stab at it!

pages/projects.tsx Outdated Show resolved Hide resolved
pages/ucla.tsx Outdated Show resolved Hide resolved
pages/projects.tsx Outdated Show resolved Hide resolved
package-lock.json Outdated Show resolved Hide resolved
@mattxwang
Copy link
Member

More of a note for reproducibility: could we document how the fixtures were generated somewhere? I imagine when the APIs get updated, etc. we'll need to regenerate them, and that would be done by people who probably didn't work on this initial PR.

@matthewcn56
Copy link
Member

More of a note for reproducibility: could we document how the fixtures were generated somewhere? I imagine when the APIs get updated, etc. we'll need to regenerate them, and that would be done by people who probably didn't work on this initial PR.

Sure, this can definitely be done by adding it into a README as well as into our design doc for this project! Can you add that in as well to the README @JCamyre ?

README.md Outdated Show resolved Hide resolved
Copy link
Member

@matthewcn56 matthewcn56 left a comment

Choose a reason for hiding this comment

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

Thanks for doing this again Joseph, I think that using the fixtures as failsafes really helps prevent errors!

However, I think that we should just keep in the failsafe for octokit errors but not write to the json file everytime that projectRequest gets ran, and we should NOT write to the fixture files during production.

The main reason why we tried doing the writeJson while grabbing the OctoKit data was to ensure that data was as up-to-date as possible. Normallyy, ISR handles this, but ISR wasn't behaving the way we wanted it to. Upon closer inspection, our netlify plugin for next.js just had to be updated to support ISR!

With ISR properly working, we can drop the writing to JSON files and maybe just make a github action that updates the fixtures everytime something gets pushed to main (which can be scheduled and prob another PR.)

So sorry for making you do extra work but we can def keep the main gist of your writeJSON code for other stuff in the future!

@matthewcn56
Copy link
Member

Closed as with updates to Next.js 12, revalidation should work properly

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Working on ucla-opensource page! Reducing API Hits And Adding In Failsafes For If Hit API Limit
3 participants