unc-csxl / csxl.unc.edu

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

Angular v17 Upgrade: Organizations Feature Refactor #455

Closed ajaygandecha closed 1 month ago

ajaygandecha commented 1 month ago

This major pull request refactors the entire organizations feature to the new standards of Angular v17. This pull request also rewrites most of the functionality to be more concise and readable, following the best conventions for future students in COMP 423 and 393, using everything we have learned so far. This is the first such refactor, and all of the features will go through the same refactor eventually.

Note: Initial thoughts for this PR noted in #454.

Also, I am adding @jadekeegan and @atoneyd to this PR as reviewers - you all do not need to submit a code review for this, however since this is a major refactor to the feature we worked on and this is the first refactor moving to the new Angular v17 standards, I wanted you all to get alerted! Feel free to check it out in your own time 😇✨ .

File Structure Changes

First, the file structure has been edited and simplified. This new structure will loosely be what all feature folders in the final frontend refactor will look like. Here is a comparison from the file structure before and after the refactor:

BeforeAfter
``` feature | admin | | list | | | (HTML / CSS / TS) | | (HTML / CSS / TS) | | feature-admin.service.ts | page (and so on...) | | (HTML / CSS / TS) | widgets | | sample widget (and so on...) | | | (HTML / CSS / TS) | pipe-name | | sample pipe (and so on...) | feature.service.ts | feature.models.ts | feature.modules.ts | feature-routing.modules.ts ``` ``` feature | admin | | (HTML / CSS / TS) | page (and so on...) | | (HTML / CSS / TS) | widgets | | sample widget (and so on...) | | | (HTML / CSS / TS) | pipes | | sample pipe (and so on...) | feature.service.ts | feature.models.ts | feature.modules.ts | feature-routing.modules.ts ```

This leads to a few considerations, as listed:

Reactive State Management with the new Signal<>

Currently, the service for the organizations feature returns RxJS Observable objects, the result of HTTP method calls, for which the components subscribe to. This, along with the RxObject construction (ReplaySubject + Observable), handle state and data transfer in the CSXL application.

The latest version of Angular has announced the Signal<> construction which allows us to more easily handle state in our application. There is also RxJS-Signal interoperability (in v17+) which allows us to easily convert to and from Observables to Signals.

Signals have a much more concise syntax and are significantly easier to use. In this refactor and elsewhere, Signal<T> will serve as the direct replacement to the RxObject abstraction - so, RxObject will soon be deprecated. RxOrganization and RxOrganizationList have been removed in the refactor.

Sample Component TypeScript File:

// OLD: With observables
organizations$: Observable<Organization[]>;
organizations: Organization[];
...
this.organizations$ = this.organizationService.getOrganizations();
this.organizations$.subscribe((organizations) => {
  this.organizations = organizations;
};

// NEW: With signals
organizationsSignal: Signal<Organization[]>;
organizations: Organization[];
...
this.organizationsSignal = this.organizationService.getOrganizations();
this.organizations = this.organizationsSignal();

In addition, working with signals in the HTML files much easier as well:

Sample Component HTML File:

<!-- OLD: Observables -->
<p>{{ (event$ | async).name }}</p>

<!-- NEW: Signals -->
<p>{{ event().name }}</p>

This is how we are able to combine the regular and admin services into one. The new OrganizationService has an organizations signal, which is updated when getOrganizations(), createOrganization(), updateOrganization(), and deleteOrganization() are called.

@Injectable({ providedIn: 'root' })
export class NewOrganizationService {

  private organizationsSignal: WritableSignal<Organization[]> = signal([]);
  organizations = this.organizationsSignal.asReadonly();
  ...
  createOrganization(organization: Organization): Signal<Organization | undefined> {
    return toSignal(
      this.http.post<Organization>('/api/organizations', organization)
        .pipe(tap((organization) => this.organizationsSignal.update((organizations) => [...organizations, organization])))
      );
  }

My original goal was to remove Observable<> entirely outside of the service layer, and leave Signal<> as the only component abstraction. However, after a lot of research, experimentation, and debugging, I found that this is not feasible. The subscribe() functionality of observables and other more advanced features of RxJS are a necessity, so all of these service functions still ultimately return Observable<Organization>. Even though Angular v17 includes Rx <-> Signal interpolation and conversion functions, this is not necessarily a one-to-one conversion as you might expect.

This refactor also reduces the dependency on resolvers, as some of these resolvers have been replaced with signals, especially where reactive data is helpful.

Angular v17 HTML Changes

One interesting update and convention change in the Angular v17 version is how the built-in ngFor and ngIf directives work. They have added new, built-in control flow into the HTML, which is the new standard. The new standard helps to limit the number of operations needed to modify the DOM (so more efficient), and may be more readable. Here is an example of the old if-else statement vs the new one in the HTML file:

<!-- OLD (Pre Angular v17) -->

<p *ngIf="authenticated; else showAlert">Logged in!</p>
<ng-template #showAlert>
  <p>Not logged in!</p>
</ng-template

<!-- NEW (Angular v17+) (yes, this is HTML code!) -->
@if (authenticated) {
  <p>Logged in!</p>
} @else {
  <p>Not logged in!</p>
}

This supports @if, @else if, and @else if-else-if-else statements, @for for for-loops, and @switch for switch-case statements. Since this is likely the convention we will be moving towards in Angular v17, I began making this conversion in the HTML.

Note that the Material upgrade refactor will occur after all of the features are refactored, so the HTML / CSS will be revisited again.

@KrisJordan, one annoying problem is the formatting with the linter with these new structures. We might need to check the settings to adjust.

General Code Improvements and Other Changes

Future Considerations

KrisJordan commented 1 month ago

For what it's worth, I am in favor of merging and deploying these iterations as they complete feature-by-feature rather than as a big bang.