Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: using subscription lifecycle or take(1) where necessary #2495

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down Expand Up @@ -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))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Finite observable; upstream emits once.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you want me to revert this or any other similar change in this PR if it is not breaking anything?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to capture what I was saying offline - I think the readability is key. Making double sure a subscription is cleaned up is fine as long as we're not making the code significantly harder to follow. I'd put take in the former camp, but subscription lifecycle (in its current form) in the latter.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just my 2c though, up to you guys.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay. let me update

.subscribe(() => {
this.dataLoading = false;
this.cdr.detectChanges();
});
}
}
12 changes: 8 additions & 4 deletions projects/components/src/select/select-group.component.ts
Original file line number Diff line number Diff line change
@@ -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';
Expand All @@ -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: `
<div class="select-group-container">
Expand All @@ -19,10 +20,13 @@ export class SelectGroupComponent implements AfterContentInit {
@ContentChildren(SelectComponent)
private readonly selectChildren!: QueryList<SelectComponent<unknown>>;

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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

query list is an angular component-local component. I would assume it already completes on component destroy.

.pipe(observeOn(asyncScheduler))
.subscribe(list => this.setPositions(list))
);
}

private setPositions(list: QueryList<SelectComponent<unknown>>): void {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -67,7 +67,8 @@ export class TableCsvDownloaderService {
fileName: fileName,
dataSource: of(content)
});
})
}),
take(1)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this one's good. We have no guarantees about what our caller will give us.

)
.subscribe();
}
Expand Down
28 changes: 15 additions & 13 deletions projects/components/src/table/table.component.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<SortedColumn<TableColumnConfigExtended>> => sort !== undefined)
)
.subscribe(sort => this.updateSort(sort));
this.subscriptionLifecycle.add(
combineLatest([this.activatedRoute.queryParamMap, this.columnConfigs$])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

two finite observables will produce a finite observable.

.pipe(
map(([queryParamMap, columns]) => this.sortDataFromUrl(queryParamMap, columns)),
filter((sort): sort is Required<SortedColumn<TableColumnConfigExtended>> => sort !== undefined)
)
.subscribe(sort => this.updateSort(sort))
);
}

public onLayoutChange(): void {
Expand Down Expand Up @@ -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(() => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also finite. The data source's disconnect method completes this (however this is questionable; that means it cannot be connect'ed again).

this.tableService.updateFilterValues(this.columnConfigsSubject.value, this.dataSource!); // Mutation! Ew!
})
);
this.filtersSubject.next(this.filters || []);
this.queryPropertiesSubject.next(this.queryProperties || {});

Expand Down Expand Up @@ -928,7 +930,6 @@ export class TableComponent
public showEditColumnsModal(): void {
this.columnConfigs$
.pipe(
take(1),
switchMap(
availableColumns =>
this.modalService.createModal<TableEditColumnsModalConfig, TableColumnConfigExtended[]>({
Expand All @@ -941,7 +942,8 @@ export class TableComponent
defaultColumns: this.columnDefaultConfigs ?? []
}
}).closed$
)
),
take(1)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

closed$ won't emit more than once.

)
.subscribe(editedColumnConfigs => {
this.saveTablePreferences(editedColumnConfigs);
Expand Down
8 changes: 5 additions & 3 deletions projects/dashboards/src/widgets/widget-renderer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,11 @@

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());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the api observables are all tied to lifecycle already.

this.api.change$.pipe(takeUntil(this.destroyed$)).subscribe(() => this.onModelChange());
this.api.timeRangeChanged$
.pipe(takeUntil(this.destroyed$))
.subscribe(timeRange => this.onTimeRangeChange(timeRange));

Check warning on line 35 in projects/dashboards/src/widgets/widget-renderer.ts

View check run for this annotation

Codecov / codecov/patch

projects/dashboards/src/widgets/widget-renderer.ts#L35

Added line #L35 was not covered by tests
}

public ngOnDestroy(): void {
Expand Down
Original file line number Diff line number Diff line change
@@ -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';
Expand All @@ -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: `
<div *ngIf="this.visualizationRequest$ | async as currentVisualization" class="query-editor">
<ht-explore-query-series-group-editor
Expand Down Expand Up @@ -94,14 +94,19 @@ export class ExploreQueryEditorComponent implements OnChanges, OnInit {

public readonly visualizationRequest$: Observable<ExploreVisualizationRequest>;

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 => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

viz request is already tied to lifecycle

this.visualizationRequestChange.emit(query);
})
);
}

public ngOnChanges(changeObject: TypedSimpleChanges<this>): void {
Expand Down
Original file line number Diff line number Diff line change
@@ -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';
Expand All @@ -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: `
<div class="group-by-container" *htLetAsync="this.currentGroupByExpression$ as currentGroupByExpression">
<div class="group-by-input-container">
Expand Down Expand Up @@ -72,7 +73,10 @@ export class ExploreQueryGroupByEditorComponent implements OnChanges {
public readonly currentGroupByExpression$: Observable<AttributeExpressionWithMetadata | undefined>;
public readonly groupByAttributeOptions$: Observable<SelectOption<AttributeMetadata | undefined>[]>;

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))
);
Expand All @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one's good, it's a bug. But rather than wrapping this subscription in a lifecycle, can we complete the subject instead?

.pipe(
filter(expression => this.isValidExpressionToEmit(expression)),
debounceTime(500),
map(expression => (isEmpty(expression) ? undefined : omit(expression, 'metadata')))
)
.subscribe(this.groupByExpressionChange)
);
}

public ngOnChanges(changeObject: TypedSimpleChanges<this>): void {
Expand Down
Original file line number Diff line number Diff line change
@@ -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';
Expand Down Expand Up @@ -43,6 +43,7 @@ import { TimelineCardContainerComponent } from './container/timeline-card-contai
</ng-template>
`,
styleUrls: ['./timeline-card-list.component.scss'],
providers: [SubscriptionLifecycle],
changeDetection: ChangeDetectionStrategy.OnPush
})
export class TimelineCardListComponent implements AfterContentInit {
Expand All @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

query list should be ok

.pipe(map(list => list.toArray()))
.subscribe(allCards => {
this.allCards = allCards;
this.buildItems(allCards);
})
);
}

public readonly isSelectedCard = (card: TimelineCardContainerComponent, selectedIndex?: number): boolean =>
Expand Down
Original file line number Diff line number Diff line change
@@ -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';
Expand All @@ -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: `
<div class="application-aware-dashboard" [style.padding.px]="this.padding">
<hda-dashboard
Expand Down Expand Up @@ -52,7 +53,10 @@ export class ApplicationAwareDashboardComponent implements OnDestroy, OnChanges
public widgetSelection?: object;
private readonly destroyDashboard$: Subject<void> = 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();
Expand All @@ -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(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the takeuntil already accomplishes this

this.timeRangeService
.getTimeRangeAndChanges()
.pipe(takeUntil(this.destroyDashboard$))
.subscribe(timeRange => dashboard.setTimeRange(timeRange))
);

this.dashboardReady.emit(dashboard);
}
Expand Down