refactor: using subscription lifecycle or take(1) where necessary #2495
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2495 +/- ##
=======================================
Coverage 83.06% 83.06%
=======================================
Files 925 925
Lines 20610 20622 +12
Branches 3254 3254
=======================================
+ Hits 17119 17130 +11
- Misses 3372 3373 +1
Partials 119 119
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Unit Test Results 4 files 316 suites 49m 23s ⏱️ Results for commit ecfadf0. |
| }); | ||
| this.fileDownloadService | ||
| .downloadAsText(metadata) | ||
| .pipe(take(1)) |
There was a problem hiding this comment.
Finite observable; upstream emits once.
There was a problem hiding this comment.
do you want me to revert this or any other similar change in this PR if it is not breaking anything?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Just my 2c though, up to you guys.
There was a problem hiding this comment.
Okay. let me update
| .pipe(observeOn(asyncScheduler)) | ||
| .subscribe(list => this.setPositions(list)); | ||
| this.subscriptionLifecycle.add( | ||
| queryListAndChanges$(this.selectChildren) |
There was a problem hiding this comment.
query list is an angular component-local component. I would assume it already completes on component destroy.
| @@ -67,7 +67,8 @@ export class TableCsvDownloaderService { | |||
| fileName: fileName, | |||
| dataSource: of(content) | |||
| }); | |||
| }) | |||
| }), | |||
| take(1) | |||
There was a problem hiding this comment.
this one's good. We have no guarantees about what our caller will give us.
| ) | ||
| .subscribe(sort => this.updateSort(sort)); | ||
| this.subscriptionLifecycle.add( | ||
| combineLatest([this.activatedRoute.queryParamMap, this.columnConfigs$]) |
There was a problem hiding this comment.
two finite observables will produce a finite observable.
| this.tableService.updateFilterValues(this.columnConfigsSubject.value, this.dataSource!); // Mutation! Ew! | ||
| }); | ||
| this.subscriptionLifecycle.add( | ||
| this.dataSource?.loadingStateChange$.subscribe(() => { |
There was a problem hiding this comment.
also finite. The data source's disconnect method completes this (however this is questionable; that means it cannot be connect'ed again).
| this.visualizationRequestChange.emit(query); | ||
| }); | ||
| this.subscriptionLifecycle.add( | ||
| this.visualizationRequest$.subscribe(query => { |
There was a problem hiding this comment.
viz request is already tied to lifecycle
| ) | ||
| .subscribe(this.groupByExpressionChange); | ||
| this.subscriptionLifecycle.add( | ||
| this.groupByExpressionsToEmit |
There was a problem hiding this comment.
This one's good, it's a bug. But rather than wrapping this subscription in a lifecycle, can we complete the subject instead?
| this.buildItems(allCards); | ||
| }); | ||
| this.subscriptionLifecycle.add( | ||
| queryListAndChanges$(this.cards) |
There was a problem hiding this comment.
query list should be ok
| .getTimeRangeAndChanges() | ||
| .pipe(takeUntil(this.destroyDashboard$)) | ||
| .subscribe(timeRange => dashboard.setTimeRange(timeRange)); | ||
| this.subscriptionLifecycle.add( |
There was a problem hiding this comment.
the takeuntil already accomplishes this
| 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()); |
There was a problem hiding this comment.
the api observables are all tied to lifecycle already.
Description
Please include a summary of the change, motivation and context.
Testing
Please describe the tests that you ran to verify your changes. Please summarize what did you test and what needs to be tested e.g. deployed and tested helm chart locally.
Checklist:
Documentation
Make sure that you have documented corresponding changes in this repository or hypertrace docs repo if required.