unc-csxl / csxl.unc.edu

CS Experience Labs' web application.
https://csxl.unc.edu
MIT License
9 stars 7 forks source link

Create Frontend Pagination Abstraction #456

Closed ajaygandecha closed 1 month ago

ajaygandecha commented 1 month ago

This pull request creates a new frontend abstraction for pagination. The abstraction supports simple pagination, as well as pagination where the HTTP response model does not match the type of model stored in each page. The abstraction makes strong use of generic types so that the feature is flexible to be used across the site with minimal localization.

This pull request does not implement the abstraction in any features. I was planning on utilizing this in the events feature refactor.

Again, I am adding @jadekeegan and @atoneyd to this PR, since we have all been working on pagination for quite a while. No need to complete CRs here! You all should not be bothered with notifications after this PR and the events refactor PR come in!

Usage

This PR adds the Paginator<T, Params> class, which is the main "service" tool responsible for pagination. This paginator object has two generic types, where T is for the type of object being paginated, and Params specifies the specific type of parameters used for pagination. This is useful for certain features such as the events feature, where EventPaginationParams has a different shape than PaginationParams to account for date ranges. The object's constructor takes in an API endpoint as a string, which is the API endpoint to be called every time a new page loads.

This is an example usage for a hypothetical organization paginator:

let paginator: Paginator<Organization, PaginationParams> = new Paginator<>('api/organizations');

Pages are loaded with the .loadPage(params) function, which takes in a params object corresponding with the generic type Params provided in the paginator's construction. This is an example usage of loading the next page for the paginator defined above:

paginator.loadPage(params);

When a page is loaded, a signal (Angular v17 feature, analogous to RxObject) called page is updated inside the paginator object. We can easily access the data like so:

paginator.page(); // Loads the current page.

The loadPage() function is more powerful. There are many cases in the CSXL site where the response model from the API is not the same as the model of data we are trying to store in pages. This is most notable in the events feature. The API endpoint for events return data in the EventJson format, where dates are codified as strings. Then, in the frontend, this is converted to standard Event format. This convention is used so that dates are converted properly.

The loadPage() function can account for this. The function has an optional generic type parameter called APIType, which by default is just T. This generic type signals the type of the response rather than the type of data stored in the page. This can be different from T in cases where they are not the same.

In addition, this defines a type alias called PaginationOperatorFunction<APIType, T, Params>, which is just an operator function that helps to convert Paginated<APIType, Params> objects to Paginated<T, Params>. This alias just helps the code to be a bit more readable, and is defined as follows:

type PaginationOperatorFunction<APIType, T, Params> = OperatorFunction<Paginated<APIType, Params>, Paginated<T, Params>>;

So, .loadPage<APIType>(params, operator) can take in an operator function of typePaginationOperatorFunction` which processes the conversion.

Here is the example usage for events:

let paginator: Paginator<Event, EventPaginationParams> = new Paginator<>('/api/event');
paginator.loadPage<EventJson>(params, eventJsonToEventOperatorFunction);
paginator.page(); // Returns the loaded page, in type `Paginated<T>`
ajaygandecha commented 1 month ago

@KrisJordan I just made a refactor. I removed the PaginationOperatorFunction as described above, because this converts from Paginated<APIType, Params> to Paginated<T, Params>. This means that a new function would need to be created for each type of pagination, which did not seem to be the best solution.

Instead, now .loadJson() just takes in an optional function of type (_: APIType) => T, which is the same type signature as something like parseEventJson. This means that the usage is truly as described, where we could just call:

let paginator: Paginator<Event, EventPaginationParams> = new Paginator<>('/api/event');
paginator.loadPage<EventJson>(params, parseEventJson);
paginator.page(); // Returns the loaded page, in type `Paginated<T>`

The .loadJson() function now completes the conversion between Paginated types, so that is abstracted out from the user.

ajaygandecha commented 1 month ago

@KrisJordan thank you for the review here! I completely agree - while the original intention was to have full flexibility with generics, you are right that there is really only two use cases, so this is probably overkill (and hampers code readability). I really like the solution you came up with, so I will refactor to get to that point!

Also, I agree about this abstraction returning observables instead of having a signal - it is probably best that we standardize to services containing the signal (the replacement for RxObject), as we have done in services previously. In the service, we can subscribe or tap into the value returned from this abstraction's .loadPage() function, and that will work better (and probably be more readable).

Will refactor and then request review again!

ajaygandecha commented 1 month ago

Alright @KrisJordan, I performed an initial refactor! What bothered me a little was the duplicated code between Paginator and TimeRangePaginator, so I made the initial paginator an abstract class called PaginatorAbstraction, did not export it (so it is not available outside of the file), and then exported implementations:

export class Paginator<T> extends PaginatorAbstraction<T, PaginationParams> {}

export class TimeRangePaginator<T> extends PaginatorAbstraction<T, TimeRangePaginationParams> {}

If you think that just having the duplicate implementations is fine, I can remove the abstract class and just have the same code for Paginator and TimeRangePaginator with the types replaced! Trade-offs of abstraction vs. readability 😄

(Also I am open to suggestions of a better name for PaginationAbstraction!)

ajaygandecha commented 1 month ago

If we want to simplify Paginated<T, Params> to just Paginated<T> as well, we can by creating two separate pagination objects, like Paginated<T> and TimeRangePaginated<T>.

ajaygandecha commented 1 month ago

Will merge, but further iteration on this will be done!