Category: angular

Below we have some problem code, this came from a real codebase (names changed here to protect the original code); this code just stinks. I’ve removed the imports for clarity.

This is certainly a memory jogger for me on how bad things can get. :/ The walk through is just below this code.

@Injectable({ providedIn: 'root' })
export class SubjectSignalDataService implements OnDestroy {
  readonly #subs = new Subscription();
  readonly #loading = signal<boolean>(true);
  readonly #subjects = signal<ISubject[]>([]);
  readonly #clusters = signal<ISubjectWithCluster[]>([]);

  // Exposed computed streams
  itemsList = computed(() =>
    this.#subjects().map(item => this.#attachPrimaryMember(item))
  );
  isInitialized = computed(() => !this.#loading());

  constructor(
    private hubA: HubServiceA,
    private hubB: HubServiceB,
    private profile: ProfileService
  ) {
    this.#subs.add(
      this.hubA.payloadStream$.subscribe((payload: IDataPayload) => {
        const mapped = this.#mapRecords(payload.sources);
        this.#subjects.set(mapped);
        this.#loading.set(false);
      })
    );

    this.#subs.add(
      this.hubB.dataWithClusters$.subscribe(
        (clusters: ISubjectWithCluster[]) => {
          const filtered = clusters.filter(
            c => c.ownerId === this.profile.currentUser().id
          );
          this.#clusters.set(filtered);
        }
      )
    );
  }

  ngOnDestroy() {
    this.#subs.unsubscribe();
  }

  #mapChats(chats: Record<string, IChat>): IChat[] {
    return Object.values(chats).map(c => ({
      chatId: c.chatId,
      channel: c.channel,
      startedAt: new Date(c.startedAt),
      endedAt: c.endedAt ? new Date(c.endedAt) : null,
      participants: c.participants,
      metadata: c.metadata,
    }));
  }

  #mapRecords(sources: Record<string, ISubject>): ISubject[] {
    return Object.values(sources).map(src => ({
      ...src,
      clusters: this.#mapChats(src.clusters as any),
    } as ISubject));
  }

  #attachPrimaryMember(item: ISubject): ISubject {
    const cluster = this.#clusters().find(c => c.id === item.id);
    const primary = cluster?.items.find(i => i.id === cluster.primaryItemId);
    return { ...item, primaryMember: primary?.member };
  }
}

Screenshots from VS code

My Walk Through of the Bad Code

Class declaration

Subscription field

Then we have readonly #subs = new Subscription(). Whenever I see a raw Subscription in a service, I assume something down below is wrong—manual subscription management in a singleton is almost always a code smell.

Private signals

Next come the private signals: #subject, #clusters, #loading. My understanding is that signals are meant to drive template reactivity—so keeping them private in a service feels off. Why not just expose Observables?

Computed streams

After that, we see the exposed computed fields (itemsList and isInitialised). These are hooked to the private signals, so they “work,” but computing inside the service like this (based on data set by hidden subscriptions) is unnecessary coupling.

Constructor subscriptions

In the constructor, two .subscribe(…) calls silently wire up hubA.payLoadStream$ and hubB.dataWithClusters$. Hiding subscriptions here makes it hard to reason about when or where data flows. I’d rather expose the raw streams and subscribe in a clear, visible component (e.g. AppComponent) after the service is instantiated.

ngOnDestroy

Here’s the ngOnDestroy() that unsubscribes #subs. But as noted above, in a root singleton this hook never actually fires—so it’s dead code.

Private helper methods

Finally, the private transformers (#mapChatsAndRecords, #attachPrimaryMember) do the data enrichment. Aside from their own complexity, tucking all this logic behind signals and subscriptions makes the service hard to follow.

Overall, the service mixes concerns—manual RxJS subscriptions, private signals, and computed logic—in a way that’s neither clear nor maintainable. I’d suggest refactoring toward plain Observables, exposing transformation pipelines, and handling subscription lifecycles in components rather than in a singleton service.

The Way I Would Approach This Code

Service with just signals in (still needs more work)

@Injectable({ providedIn: 'root' })
export class SubjectSignalService {
  readonly #loading = signal<boolean>(true);
  readonly #subjects = signal<ISubject[]>([]);
  readonly #clusters = signal<ISubjectWithCluster[]>([]);

  isLoading = computed(() => this.#loading());
  isReady = computed(() => !this.#loading());
  subjects = computed(() => this.#subjects().map(sub => this.#attachMain(sub)));

  setSubjects(subjects: ISubject[]): void {
    this.#subjects.set(subjects);
    this.#loading.set(false);
  }

  setClusters(clusters: ISubjectWithCluster[]): void {
    this.#clusters.set(clusters);
  }

  #attachMain(subject: ISubject): ISubject {
    const cluster = this.#clusters().find(c => c.id === subject.id);
    const mainItem = cluster?.items.find(item => item.id === cluster.primaryItemId);
    return {
      ...subject,
      primaryMember: mainItem?.member,
    };
  }
}

Mapping logic extracted

So far I’ve separated the observable streams from the signals, this let me move two of the private mapping methods out of this service (they simply transform incoming data).

Private signals remain

I haven’t cleaned up the private signals (#loading, #subjects, #clusters), but remember: signals replace zone.js change detection and should be public so templates can consume them.

Setter methods for data flow

I added setSubjects and setClusters methods, these are now called from the app component (or other orchestrating code) after subscribing to the extracted observable sources.

Second Service

This is the service that I have moved the observables into, note the naming convention, same name as the first service but just with “Data” appended.

@Injectable({ providedIn: 'root' })
export class SubjectSignalDataService {
  readonly #records$ = new BehaviorSubject<IDataRecord[]>([]);
  readonly #groups$ = new BehaviorSubject<IDataRecordWithGroup[]>([]);

  /** Public streams of the latest data */
  records$ = this.#records$.asObservable();
  recordsWithGroups$ = this.#groups$.asObservable();

  constructor(
    private hubA: HubServiceA,
    private hubB: HubServiceB,
    private profile: ProfileService
  ) {}

  /**
   * Stream of grouped records filtered by current user, updates internal subject.
   */
  getFilteredRecordsWithGroups$(): Observable<IDataRecordWithGroup[]> {
    return this.hubB.dataWithGroups$.pipe(
      map(arr => arr.filter(g => g.ownerId === this.profile.currentUser().id)),
      tap(arr => this.#groups$.next(arr))
    );
  }

  /**
   * Stream of transformed records, updates internal subject.
   */
  getTransformedRecords$(): Observable<IDataRecord[]> {
    return this.hubA.agentState$.pipe(
      map((state: IAgentState) => this.#mapRecords(state.sources)),
      tap(items => this.#records$.next(items))
    );
  }

  /**
   * Convert raw chat records into an array of IChat
   */
  #mapChats(chats: Record<string, IChat>): IChat[] {
    return Object.values(chats).map(c => ({
      chatId: c.chatId,
      channel: c.channel as ChannelKind,
      startedAt: new Date(c.startedAt),
      endedAt: c.endedAt ? new Date(c.endedAt) : null,
      participants: c.participants,
      metadata: c.metadata,
    }));
  }

  /**
   * Convert raw data sources into IDataRecord array
   */
  #mapRecords(sources: Record<string, IDataRecordSource>): IDataRecord[] {
    return Object.values(sources).map(src => ({
      recordId: src.sourceId,
      chats: this.#mapChats(src.chats),
      categoryId: src.categoryId ?? null,
      userId: src.userId ?? null,
      state: src.state as RecordState,
      createdOn: new Date(src.createdOn),
      mainChatId: src.mainChatId,
      tags: src.tags,
      mediaType: src.mediaType as MediaKind,
      isActive: src.isActive,
      notes: src.notes,
      primaryOwner: undefined,
    }));
  }
}

BehaviorSubjects for further chaining

BehaviorSubjects so that other features can chain or map this data downstream without relying on the service with the signal data, I think mapping of the observables and creating a service per feature for the signal data helps a lot for readability.

Screenshots from VS code with correct formatting.

Summary

I would simplify SubjectSignalService further. You do not need both isLoading and isReady if you structure your data flow correctly. Since you now call the SubjectSignalDataService observables directly in the places where data is needed, you can manage a single loading flag there. When you subscribe and receive data, set your loading flag to true; once you call the signal setters, set it back to false.

The original service misused signals in several ways. Because its private signals were initialized with empty arrays, developers elsewhere began calling toObservable() on a public signal that was still empty. I’ll write a few short posts to demonstrate these issues and show how to avoid them.

P.S. I apologise if any of the code names do not match, I tried to squeeze the creation on this post in between patting our twin babies to sleep, screams and gurgles, across several days, etc, I hope you catch my drift, if you have children, you will. 🙂