Extract helpers.ts

[MinhxNguyen7]

Summary

Extract all course/WebSOC-related functionality to the WebSOC class in $lib/websoc.ts. There should be no change in behavior

I do not sleep with OOP, but my God is this better than the garbage can that was the old helpers.ts.

Test Plan

Check that everything works. I can't get too much more specific than that.

This PR mostly touches on WebSOC data request, caching, and manipulation functionality, so checking if things load properly is the priority.

Issues

Closes #760

[EricPedley]

Yeah OOP kinda is the exact right tool for encapsulating the websoc query functions with the cache. Now we know for sure the private functions are only used within that class. LGTM! 🚀

bonus points for negative diff

[ap0nia]

I don't think it makes any sense to make one class to encapsulate private functions. You could accomplish the exact same thing by not exporting the private functions from the module. There's no benefit to OOP because instances of the websoc query class don't inherently manage any individual data, this is a perfect scenario to apply procedural programming because all you're doing is processing data in -> data out with not many side effects. (There aren't even multiple instances, it's just one).

Am I missing something?

[EricPedley]

The websoc class also has its cache, which makes sense as a private field and changes as a side effect when the public methods are called. I don't really see the downside of this approach vs a module with functions we don't export

[ap0nia]

If that's the only benefit of having this class, then I think it would be clearer if everything was static. For example:

export class Websoc {
  private static cache = {}

  public static async query() {
    // ...
  }
}

// Usage:

Websoc.query(...)

In my opinion, I just dislike this illusion of encapsulation. The constructor doesn't actually do anything unique per instantiation, and the class itself isn't used in a way that actually takes advantage of OOP; it's literally just a couple of methods that are invoked like regular functions, but now you need to say websoc.query(...) instead of queryWebsoc(...), which seems like you should be aware of the extra class logic in class Websoc (which, counterintuitively, there isn't any --aside from the cache).

I think static would be a valid compromise because it's "closer" to the level of module instantiation in that it doesn't readily support unique behavior per instance, and can exist just to encapsulate variables and mutate them freely (since imports/exports are immutable by the ES6 standard).

Also, we should migrate to Tanstack query eventually ™️, which handles rendering and caching. So the most ideal scenario in the future is that these are just procedural functions that fetch data for the library's hooks, and the library handles caching, mutations, and other server-side state altogether. (i.e. the endgoal is to not have a class).

[MinhxNguyen7]

This is equivalent to using a static class. The interface would be the same. Again, I'm not married to OOP; there's no reason a singleton is better than static here.

It should be a class because it has state and side effects. Every function accesses it. It's not really a valid point to say, "well it doesn't have that much state", because having state just sitting around in the module is unclear in the opposite direction. It's not just data in -> data out, so that shouldn't be indicated.

Also, I strongly prefer websoc.query() because of the namespace. It's much quicker to read. It makes it clear that all interfacing with WebSOC is done here and nothing else.

This is not about encapsulation or safety in some way. It's cleaner and clearer.

I'm not familiar with Tanstack. What's the advantage there?