From ecfadf0a183f2c9da71afc17fd24d0f047eb8e9e Mon Sep 17 00:00:00 2001 From: Anand Tiwary Date: Wed, 1 Nov 2023 11:44:45 -0700 Subject: [PATCH] refactor: using subscription lifecycle or take(1) where necessary --- .../download-file/download-file.component.ts | 12 +++++--- .../src/select/select-group.component.ts | 12 +++++--- .../src/table/table-csv-downloader.service.ts | 5 ++-- .../components/src/table/table.component.ts | 28 ++++++++++--------- .../dashboards/src/widgets/widget-renderer.ts | 8 ++++-- .../explore-query-editor.component.ts | 17 +++++++---- ...explore-query-group-by-editor.component.ts | 24 ++++++++++------ .../timeline-card-list.component.ts | 19 ++++++++----- .../application-aware-dashboard.component.ts | 18 ++++++++---- 9 files changed, 89 insertions(+), 54 deletions(-) diff --git a/projects/components/src/download-file/download-file.component.ts b/projects/components/src/download-file/download-file.component.ts index ec427f264..671a18fa3 100644 --- a/projects/components/src/download-file/download-file.component.ts +++ b/projects/components/src/download-file/download-file.component.ts @@ -4,6 +4,7 @@ import { ButtonSize, ButtonStyle } from '../button/button'; import { IconSize } from '../icon/icon-size'; import { DownloadFileMetadata } from './download-file-metadata'; import { FileDownloadService } from './service/file-download.service'; +import { take } from 'rxjs/operators'; @Component({ selector: 'ht-download-file', @@ -36,9 +37,12 @@ export class DownloadFileComponent { public triggerDownload(metadata: DownloadFileMetadata): void { this.dataLoading = true; - this.fileDownloadService.downloadAsText(metadata).subscribe(() => { - this.dataLoading = false; - this.cdr.detectChanges(); - }); + this.fileDownloadService + .downloadAsText(metadata) + .pipe(take(1)) + .subscribe(() => { + this.dataLoading = false; + this.cdr.detectChanges(); + }); } } diff --git a/projects/components/src/select/select-group.component.ts b/projects/components/src/select/select-group.component.ts index 54aa87e5f..723f5c525 100644 --- a/projects/components/src/select/select-group.component.ts +++ b/projects/components/src/select/select-group.component.ts @@ -1,5 +1,5 @@ import { AfterContentInit, ChangeDetectionStrategy, Component, ContentChildren, QueryList } from '@angular/core'; -import { queryListAndChanges$ } from '@hypertrace/common'; +import { SubscriptionLifecycle, queryListAndChanges$ } from '@hypertrace/common'; import { asyncScheduler } from 'rxjs'; import { observeOn } from 'rxjs/operators'; import { SelectGroupPosition } from './select-group-position'; @@ -8,6 +8,7 @@ import { SelectComponent } from './select.component'; @Component({ selector: 'ht-select-group', styleUrls: ['./select-group.component.scss'], + providers: [SubscriptionLifecycle], changeDetection: ChangeDetectionStrategy.OnPush, template: `
@@ -19,10 +20,13 @@ export class SelectGroupComponent implements AfterContentInit { @ContentChildren(SelectComponent) private readonly selectChildren!: QueryList>; + public constructor(private readonly subscriptionLifecycle: SubscriptionLifecycle) {} public ngAfterContentInit(): void { - queryListAndChanges$(this.selectChildren) - .pipe(observeOn(asyncScheduler)) - .subscribe(list => this.setPositions(list)); + this.subscriptionLifecycle.add( + queryListAndChanges$(this.selectChildren) + .pipe(observeOn(asyncScheduler)) + .subscribe(list => this.setPositions(list)) + ); } private setPositions(list: QueryList>): void { diff --git a/projects/components/src/table/table-csv-downloader.service.ts b/projects/components/src/table/table-csv-downloader.service.ts index 7c383c2f7..f67c55ed6 100644 --- a/projects/components/src/table/table-csv-downloader.service.ts +++ b/projects/components/src/table/table-csv-downloader.service.ts @@ -3,7 +3,7 @@ import { Injectable } from '@angular/core'; import { TableColumnConfigExtended } from './table.service'; import { TableRow } from './table-api'; import { CsvFileName, FileDownloadService } from '../download-file/service/file-download.service'; -import { map, switchMap } from 'rxjs/operators'; +import { map, switchMap, take } from 'rxjs/operators'; import { isEmpty, isNil, isString } from 'lodash-es'; import { Dictionary } from '@hypertrace/common'; import { NotificationService } from '../notification/notification.service'; @@ -67,7 +67,8 @@ export class TableCsvDownloaderService { fileName: fileName, dataSource: of(content) }); - }) + }), + take(1) ) .subscribe(); } diff --git a/projects/components/src/table/table.component.ts b/projects/components/src/table/table.component.ts index 41c1caa29..2e5e5e3ca 100644 --- a/projects/components/src/table/table.component.ts +++ b/projects/components/src/table/table.component.ts @@ -528,13 +528,14 @@ export class TableComponent this.downloadCsv(); }) ); - - combineLatest([this.activatedRoute.queryParamMap, this.columnConfigs$]) - .pipe( - map(([queryParamMap, columns]) => this.sortDataFromUrl(queryParamMap, columns)), - filter((sort): sort is Required> => sort !== undefined) - ) - .subscribe(sort => this.updateSort(sort)); + this.subscriptionLifecycle.add( + combineLatest([this.activatedRoute.queryParamMap, this.columnConfigs$]) + .pipe( + map(([queryParamMap, columns]) => this.sortDataFromUrl(queryParamMap, columns)), + filter((sort): sort is Required> => sort !== undefined) + ) + .subscribe(sort => this.updateSort(sort)) + ); } public onLayoutChange(): void { @@ -840,10 +841,11 @@ export class TableComponent this.dataSource?.disconnect(); this.dataSource = this.buildDataSource(); - - this.dataSource?.loadingStateChange$.subscribe(() => { - this.tableService.updateFilterValues(this.columnConfigsSubject.value, this.dataSource!); // Mutation! Ew! - }); + this.subscriptionLifecycle.add( + this.dataSource?.loadingStateChange$.subscribe(() => { + this.tableService.updateFilterValues(this.columnConfigsSubject.value, this.dataSource!); // Mutation! Ew! + }) + ); this.filtersSubject.next(this.filters || []); this.queryPropertiesSubject.next(this.queryProperties || {}); @@ -928,7 +930,6 @@ export class TableComponent public showEditColumnsModal(): void { this.columnConfigs$ .pipe( - take(1), switchMap( availableColumns => this.modalService.createModal({ @@ -941,7 +942,8 @@ export class TableComponent defaultColumns: this.columnDefaultConfigs ?? [] } }).closed$ - ) + ), + take(1) ) .subscribe(editedColumnConfigs => { this.saveTablePreferences(editedColumnConfigs); diff --git a/projects/dashboards/src/widgets/widget-renderer.ts b/projects/dashboards/src/widgets/widget-renderer.ts index 7e31235bb..40a9b1ced 100644 --- a/projects/dashboards/src/widgets/widget-renderer.ts +++ b/projects/dashboards/src/widgets/widget-renderer.ts @@ -28,9 +28,11 @@ export abstract class WidgetRenderer imp public ngOnInit(): void { this.fetchAndRunChangeDetection(); - this.api.dataRefresh$.subscribe(() => this.onDashboardRefresh()); - this.api.change$.subscribe(() => this.onModelChange()); - this.api.timeRangeChanged$.subscribe(timeRange => this.onTimeRangeChange(timeRange)); + this.api.dataRefresh$.pipe(takeUntil(this.destroyed$)).subscribe(() => this.onDashboardRefresh()); + this.api.change$.pipe(takeUntil(this.destroyed$)).subscribe(() => this.onModelChange()); + this.api.timeRangeChanged$ + .pipe(takeUntil(this.destroyed$)) + .subscribe(timeRange => this.onTimeRangeChange(timeRange)); } public ngOnDestroy(): void { diff --git a/projects/observability/src/shared/components/explore-query-editor/explore-query-editor.component.ts b/projects/observability/src/shared/components/explore-query-editor/explore-query-editor.component.ts index 8d518d08e..0179a429e 100644 --- a/projects/observability/src/shared/components/explore-query-editor/explore-query-editor.component.ts +++ b/projects/observability/src/shared/components/explore-query-editor/explore-query-editor.component.ts @@ -1,5 +1,5 @@ import { ChangeDetectionStrategy, Component, EventEmitter, Input, OnChanges, OnInit, Output } from '@angular/core'; -import { TypedSimpleChanges } from '@hypertrace/common'; +import { SubscriptionLifecycle, TypedSimpleChanges } from '@hypertrace/common'; import { Filter } from '@hypertrace/components'; import { Observable } from 'rxjs'; import { AttributeExpression } from '../../graphql/model/attribute/attribute-expression'; @@ -17,7 +17,7 @@ import { selector: 'ht-explore-query-editor', styleUrls: ['./explore-query-editor.component.scss'], changeDetection: ChangeDetectionStrategy.OnPush, - providers: [ExploreVisualizationBuilder], + providers: [ExploreVisualizationBuilder, SubscriptionLifecycle], template: `
; - public constructor(private readonly visualizationBuilder: ExploreVisualizationBuilder) { + public constructor( + private readonly visualizationBuilder: ExploreVisualizationBuilder, + private readonly subscriptionLifecycle: SubscriptionLifecycle + ) { this.visualizationRequest$ = visualizationBuilder.visualizationRequest$; } public ngOnInit(): void { - this.visualizationRequest$.subscribe(query => { - this.visualizationRequestChange.emit(query); - }); + this.subscriptionLifecycle.add( + this.visualizationRequest$.subscribe(query => { + this.visualizationRequestChange.emit(query); + }) + ); } public ngOnChanges(changeObject: TypedSimpleChanges): void { diff --git a/projects/observability/src/shared/components/explore-query-editor/group-by/explore-query-group-by-editor.component.ts b/projects/observability/src/shared/components/explore-query-editor/group-by/explore-query-group-by-editor.component.ts index 94d29ca4f..5a253fcad 100644 --- a/projects/observability/src/shared/components/explore-query-editor/group-by/explore-query-group-by-editor.component.ts +++ b/projects/observability/src/shared/components/explore-query-editor/group-by/explore-query-group-by-editor.component.ts @@ -1,5 +1,5 @@ import { ChangeDetectionStrategy, Component, EventEmitter, Input, OnChanges, Output } from '@angular/core'; -import { TypedSimpleChanges } from '@hypertrace/common'; +import { SubscriptionLifecycle, TypedSimpleChanges } from '@hypertrace/common'; import { InputAppearance, SelectOption, SelectSearchMode } from '@hypertrace/components'; import { isEmpty, isNil, omit } from 'lodash-es'; import { combineLatest, merge, Observable, of, ReplaySubject, Subject } from 'rxjs'; @@ -12,6 +12,7 @@ import { MetadataService } from '../../../services/metadata/metadata.service'; selector: 'ht-explore-query-group-by-editor', styleUrls: ['./explore-query-group-by-editor.component.scss'], changeDetection: ChangeDetectionStrategy.OnPush, + providers: [SubscriptionLifecycle], template: `
@@ -72,7 +73,10 @@ export class ExploreQueryGroupByEditorComponent implements OnChanges { public readonly currentGroupByExpression$: Observable; public readonly groupByAttributeOptions$: Observable[]>; - public constructor(private readonly metadataService: MetadataService) { + public constructor( + private readonly metadataService: MetadataService, + private readonly subscriptionLifecycle: SubscriptionLifecycle + ) { this.groupByAttributeOptions$ = this.contextSubject.pipe( switchMap(context => this.getGroupByOptionsForContext(context)) ); @@ -82,13 +86,15 @@ export class ExploreQueryGroupByEditorComponent implements OnChanges { merge(this.incomingGroupByExpressionSubject, this.groupByExpressionsToEmit) ]).pipe(map(optionsAndKey => this.resolveAttributeFromOptions(...optionsAndKey))); - this.groupByExpressionsToEmit - .pipe( - filter(expression => this.isValidExpressionToEmit(expression)), - debounceTime(500), - map(expression => (isEmpty(expression) ? undefined : omit(expression, 'metadata'))) - ) - .subscribe(this.groupByExpressionChange); + this.subscriptionLifecycle.add( + this.groupByExpressionsToEmit + .pipe( + filter(expression => this.isValidExpressionToEmit(expression)), + debounceTime(500), + map(expression => (isEmpty(expression) ? undefined : omit(expression, 'metadata'))) + ) + .subscribe(this.groupByExpressionChange) + ); } public ngOnChanges(changeObject: TypedSimpleChanges): void { diff --git a/projects/observability/src/shared/components/timeline-card-list/timeline-card-list.component.ts b/projects/observability/src/shared/components/timeline-card-list/timeline-card-list.component.ts index 7cd901d37..0c78c1f2b 100644 --- a/projects/observability/src/shared/components/timeline-card-list/timeline-card-list.component.ts +++ b/projects/observability/src/shared/components/timeline-card-list/timeline-card-list.component.ts @@ -1,5 +1,5 @@ import { AfterContentInit, ChangeDetectionStrategy, Component, ContentChildren, Input, QueryList } from '@angular/core'; -import { DateFormatMode, DateFormatOptions, queryListAndChanges$ } from '@hypertrace/common'; +import { DateFormatMode, DateFormatOptions, SubscriptionLifecycle, queryListAndChanges$ } from '@hypertrace/common'; import { ButtonVariant, ButtonStyle } from '@hypertrace/components'; import { map } from 'rxjs/operators'; import { TimelineCardContainerComponent } from './container/timeline-card-container.component'; @@ -43,6 +43,7 @@ import { TimelineCardContainerComponent } from './container/timeline-card-contai `, styleUrls: ['./timeline-card-list.component.scss'], + providers: [SubscriptionLifecycle], changeDetection: ChangeDetectionStrategy.OnPush }) export class TimelineCardListComponent implements AfterContentInit { @@ -62,13 +63,17 @@ export class TimelineCardListComponent implements AfterContentInit { public items: TimelineListItem[] = []; public allCards: TimelineCardContainerComponent[] = []; + public constructor(private readonly subscriptionLifecycle: SubscriptionLifecycle) {} + public ngAfterContentInit(): void { - queryListAndChanges$(this.cards) - .pipe(map(list => list.toArray())) - .subscribe(allCards => { - this.allCards = allCards; - this.buildItems(allCards); - }); + this.subscriptionLifecycle.add( + queryListAndChanges$(this.cards) + .pipe(map(list => list.toArray())) + .subscribe(allCards => { + this.allCards = allCards; + this.buildItems(allCards); + }) + ); } public readonly isSelectedCard = (card: TimelineCardContainerComponent, selectedIndex?: number): boolean => diff --git a/projects/observability/src/shared/dashboard/dashboard-wrapper/application-aware-dashboard.component.ts b/projects/observability/src/shared/dashboard/dashboard-wrapper/application-aware-dashboard.component.ts index 8482cb224..aa7831053 100644 --- a/projects/observability/src/shared/dashboard/dashboard-wrapper/application-aware-dashboard.component.ts +++ b/projects/observability/src/shared/dashboard/dashboard-wrapper/application-aware-dashboard.component.ts @@ -1,5 +1,5 @@ import { ChangeDetectionStrategy, Component, EventEmitter, Input, OnChanges, OnDestroy, Output } from '@angular/core'; -import { Dictionary, TimeRangeService } from '@hypertrace/common'; +import { Dictionary, SubscriptionLifecycle, TimeRangeService } from '@hypertrace/common'; import { Dashboard, ModelJson } from '@hypertrace/hyperdash'; import { TypedSimpleChanges } from '@hypertrace/hyperdash-angular/util/angular-change-object'; import { Subject } from 'rxjs'; @@ -10,6 +10,7 @@ import { GraphQlFilterDataSourceModel } from '../data/graphql/filter/graphql-fil selector: 'ht-application-aware-dashboard', styleUrls: ['./application-aware-dashboard.component.scss'], changeDetection: ChangeDetectionStrategy.OnPush, + providers: [SubscriptionLifecycle], template: `
= new Subject(); - public constructor(private readonly timeRangeService: TimeRangeService) {} + public constructor( + private readonly timeRangeService: TimeRangeService, + private readonly subscriptionLifecycle: SubscriptionLifecycle + ) {} public onDashboardReady(dashboard: Dashboard): void { this.destroyDashboard$.next(); @@ -62,10 +66,12 @@ export class ApplicationAwareDashboardComponent implements OnDestroy, OnChanges this.dashboard = dashboard; this.widgetSelection = dashboard.root; - this.timeRangeService - .getTimeRangeAndChanges() - .pipe(takeUntil(this.destroyDashboard$)) - .subscribe(timeRange => dashboard.setTimeRange(timeRange)); + this.subscriptionLifecycle.add( + this.timeRangeService + .getTimeRangeAndChanges() + .pipe(takeUntil(this.destroyDashboard$)) + .subscribe(timeRange => dashboard.setTimeRange(timeRange)) + ); this.dashboardReady.emit(dashboard); }