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

Switch to an external cloud function to store requirements JSON #889

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

Destaq
Copy link
Member

@Destaq Destaq commented Feb 15, 2024

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:

  • instead of reading from decorated-requirements.json we instead have a store that contains the requirements data that is relevant to a specific user
  • data / the store is updated when a user changes majors etc. on their profile
  • working functionality when a new user is onboarded as well

Remaining TODOs:

  • create the actual cloud function in firebase (I don't have perms for this, have been running locally)
  • somehow (GitHub Action?) make it so that the JSON file which the cloud function it itself references is updated whenever 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)
  • update URL in src/requirements/filter-from-api.ts to match TODO 1

Test 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.)

  1. Make a new express app somewhere and add the following in index.js
const express = require("express");
const fs = require("fs").promises; // Use fs promises API for simpler async handling
const path = require("path");

const app = express();
const port = 3000;

// Enable CORS for all routes
app.use((req, res, next) => {
    res.header("Access-Control-Allow-Origin", "*");
    next();
});

// Function to read and optionally filter the JSON data
async function readAndFilterData(filterParams) {
    const filePath = path.join(__dirname, "decorated-requirements.json");
    const jsonData = await fs.readFile(filePath, "utf8");
    const data = JSON.parse(jsonData);

    // Always include "university": "UNI"
    const filteredData = { university: data.university };

    // Filter based on provided query parameters
    ["grad", "minor", "major", "college"].forEach((param) => {
        if (filterParams[param]) {
            // If the parameter exists in the query string, and is found in data, add it to filteredData
            const paramValues = filterParams[param].split(",");
            filteredData[param] = {};
            paramValues.forEach((value) => {
                if (data[param] && data[param].hasOwnProperty(value.trim())) {
                    filteredData[param][value.trim()] =
                        data[param][value.trim()];
                }
            });
        } else {
            // If the parameter doesn't exist, or not found, include everything under it
            // *unless* the parameter is "skip-this", in which case we don't include it at all.
            if (param !== "skip-this") {
                filteredData[param] = data[param];
            }
        }
    });

    return filteredData;
}

// Express route to handle the GET requests
app.get("/requirements", async (req, res) => {
    try {
        const filteredData = await readAndFilterData(req.query);
        res.json(filteredData);
    } catch (error) {
        res.status(500).json({ error: "Failed to read or filter data" });
    }
});

app.listen(port, () => console.log(`Server running on port ${port}`));
  1. At the same level as the above file, copy over the decorated-requirements.json file.
  2. node index.js
  3. Go to src/requirements/filter-from-api.ts and update BASE_URL to http://localhost:3000/requirements.
  4. Now you can test things properly.

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.

  • This is supposed to make things go faster by returning a smaller requirements json file (true), but isn't there inherent lag in fetching?
  • Since we have to wait until the store + site is loaded before we can call the server (since we need to know what major the user is for example), there is a short flicker (probably even bigger once we are not having a local server) as the requirements sidebar panel is filled with data (i.e. it goes from having 0/0 requirements satisfied for your college to everything else)
  • Because we always only see a subset of the requirements, when you are onboarding / editing your major or minor or college we may have to repeatedly ping the server, and this is slow. It also prevents us from properly checking for some errors (see my comment later in this PR).
  • It will be a pain to update the cloud function whenever the base requirements graph is regenerated
  • We are passing around the requirements json file a bit within functions (this is necessary, I minimized it as much as I could but it still happened a few places) which might be inefficient
  • We introduce one more thing that could break the app, if the firebase cloud function goes offline then this won't function whereas currently that isn't an issue

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.

@Destaq Destaq requested a review from a team as a code owner February 15, 2024 01:31
@Destaq Destaq linked an issue Feb 15, 2024 that may be closed by this pull request
@dti-github-bot
Copy link
Member

[diff-counting] Significant lines: 294.

Copy link
Contributor

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
Copy link
Member Author

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.

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.

Reduce App Size with API to Send Requirement Data to Clients
2 participants