-
Notifications
You must be signed in to change notification settings - Fork 9
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
Switch to an external cloud function to store requirements JSON #889
base: main
Are you sure you want to change the base?
Conversation
[diff-counting] Significant lines: 294. |
Visit the preview URL for this PR (updated for commit 46d0b70): https://cornelldti-courseplan-dev--pr889-simon-use-requiremen-xnpx53sl.web.app (expires Sat, 16 Mar 2024 01:32:24 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 6d4e0437c4559ed895272bbd63991394f1e0e933 |
@@ -119,16 +113,19 @@ export default defineComponent({ | |||
} | |||
return false; | |||
}, | |||
// FIXME: the requirement graph is not generated for the chosen major / minor / grad |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This wasn't super clear. Essentially what I mean here is that if you click to pick a new major (or college or minor or grad), we don't fetch from the cloud function to get the requirements for that major. Doing so would take say 0.5 seconds and so the warning would flash for 0.5 seconds, as the current requirement json has no information about whatever new choice you made and would hence think it is invalid.
Summary
In #705 we talked about using a firebase cloud function to send relevant sub-parts of the decorate requirements json file so that initial page speed is quicker. This PR takes steps towards implementing this:
decorated-requirements.json
we instead have a store that contains the requirements data that is relevant to a specific userRemaining TODOs:
npm run req-gen
is done (aka when we have a new major or whatnot then we need to update the base requirements JSON file)src/requirements/filter-from-api.ts
to match TODO 1Test Plan
Buckle up this is going to be long.
First up you need to make a local server that you can call, since we don't have a firebase cloud function setup yet. (Once we have it setup then this will be quite easier to test.)
index.js
decorated-requirements.json
file.node index.js
src/requirements/filter-from-api.ts
and updateBASE_URL
tohttp://localhost:3000/requirements
.Note that if you want to test against a locally run firebase cloud function, you'll have to message me — took a lot of configuring and would be overwhelming to put things here.
Notes
I really don't like this PR for a number of reasons. It would bring me great joy if we did not merge it.
Note that the tests are failing because currently the virtual machine runner can't access the cloud function server since it's localhost.
Breaking Changes
Lots of internal reworking to prevent there from being cyclical dependencies. Means that some of the internal util "APIs" have changed.