Introduce the concept of Finalizable to tear down services.
This will allow services and AppContext to be properly cleared
when necessary.
This is in the interest of getting rid of globals. In the parent change,
it was discovered that some services don't clean up nicely hence this
change.
Additionally, as we will allow creating and removing of certain services
(e.g. ChangeService and CommentsService) they will also need the ability
to be cleaned up.
In addition, clean up how we register services for tests and clean up
after tests.
Because we now subscribe more often as we are recreating Services for
each test, subscriptions were leaking and there would be
stack-overflow-errors in the subscribe logic. To address this, move away
from using disconnected$ and instead rely on .unsubscribe() for
subscriptions;
Google-Bug-Id: b/206459178
Change-Id: I93ee6933ccc68728e1de174e1daac52cc4b83dc7
diff --git a/.bazelrc b/.bazelrc
index 6ccd56a..3f9335c 100644
--- a/.bazelrc
+++ b/.bazelrc
@@ -18,7 +18,7 @@
build --experimental_worker_multiplex=false
test --build_tests_only
-test --test_output=errors
+test --test_output=all
test --java_toolchain=//tools:error_prone_warnings_toolchain_java11
import %workspace%/tools/remote-bazelrc
diff --git a/polygerrit-ui/app/elements/admin/gr-create-change-dialog/gr-create-change-dialog.ts b/polygerrit-ui/app/elements/admin/gr-create-change-dialog/gr-create-change-dialog.ts
index e220e7d..037e11f 100644
--- a/polygerrit-ui/app/elements/admin/gr-create-change-dialog/gr-create-change-dialog.ts
+++ b/polygerrit-ui/app/elements/admin/gr-create-change-dialog/gr-create-change-dialog.ts
@@ -29,15 +29,14 @@
} from '../../../types/common';
import {InheritedBooleanInfoConfiguredValue} from '../../../constants/constants';
import {getAppContext} from '../../../services/app-context';
-import {Subject} from 'rxjs';
import {serverConfig$} from '../../../services/config/config-model';
-import {takeUntil} from 'rxjs/operators';
import {formStyles} from '../../../styles/gr-form-styles';
import {sharedStyles} from '../../../styles/shared-styles';
import {LitElement, PropertyValues, css, html} from 'lit';
import {customElement, property, query, state} from 'lit/decorators';
import {BindValueChangeEvent} from '../../../types/events';
import {fireEvent} from '../../../utils/event-util';
+import {subscribe} from '../../lit/subscription-controller';
const SUGGESTIONS_LIMIT = 15;
const REF_PREFIX = 'refs/heads/';
@@ -78,8 +77,6 @@
private readonly restApiService = getAppContext().restApiService;
- disconnected$ = new Subject();
-
constructor() {
super();
this.query = (input: string) => this.getRepoBranchesSuggestions(input);
@@ -89,17 +86,12 @@
super.connectedCallback();
if (!this.repoName) return;
- serverConfig$.pipe(takeUntil(this.disconnected$)).subscribe(config => {
+ subscribe(this, serverConfig$, config => {
this.privateChangesEnabled =
config?.change?.disable_private_changes ?? false;
});
}
- override disconnectedCallback() {
- this.disconnected$.next();
- super.disconnectedCallback();
- }
-
static override get styles() {
return [
formStyles,
diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts
index b01de6a..a84b33d 100644
--- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts
+++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts
@@ -14,6 +14,7 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
+import {Subscription} from 'rxjs';
import '@polymer/paper-tabs/paper-tabs';
import '../../../styles/gr-a11y-styles';
import '../../../styles/gr-paper-styles';
@@ -181,9 +182,7 @@
fireTitleChange,
} from '../../../utils/event-util';
import {GerritView, routerView$} from '../../../services/router/router-model';
-import {takeUntil} from 'rxjs/operators';
import {aPluginHasRegistered$} from '../../../services/checks/checks-model';
-import {Subject} from 'rxjs';
import {debounce, DelayedTask, throttleWrap} from '../../../utils/async-util';
import {Interaction, Timing} from '../../../constants/reporting';
import {ChangeStates} from '../../shared/gr-change-status/gr-change-status';
@@ -609,7 +608,7 @@
];
}
- disconnected$ = new Subject();
+ private subscriptions: Subscription[] = [];
private replyRefitTask?: DelayedTask;
@@ -627,25 +626,31 @@
override ready() {
super.ready();
- aPluginHasRegistered$.pipe(takeUntil(this.disconnected$)).subscribe(b => {
- this._showChecksTab = b;
- });
- routerView$.pipe(takeUntil(this.disconnected$)).subscribe(view => {
- this.isViewCurrent = view === GerritView.CHANGE;
- });
- drafts$.pipe(takeUntil(this.disconnected$)).subscribe(drafts => {
- this._diffDrafts = {...drafts};
- });
- preferenceDiffViewMode$
- .pipe(takeUntil(this.disconnected$))
- .subscribe(diffViewMode => {
+ this.subscriptions.push(
+ aPluginHasRegistered$.subscribe(b => {
+ this._showChecksTab = b;
+ })
+ );
+ this.subscriptions.push(
+ routerView$.subscribe(view => {
+ this.isViewCurrent = view === GerritView.CHANGE;
+ })
+ );
+ this.subscriptions.push(
+ drafts$.subscribe(drafts => {
+ this._diffDrafts = {...drafts};
+ })
+ );
+ this.subscriptions.push(
+ preferenceDiffViewMode$.subscribe(diffViewMode => {
this.diffViewMode = diffViewMode;
- });
- changeComments$
- .pipe(takeUntil(this.disconnected$))
- .subscribe(changeComments => {
+ })
+ );
+ this.subscriptions.push(
+ changeComments$.subscribe(changeComments => {
this._changeComments = changeComments;
- });
+ })
+ );
}
constructor() {
@@ -727,7 +732,10 @@
}
override disconnectedCallback() {
- this.disconnected$.next();
+ for (const s of this.subscriptions) {
+ s.unsubscribe();
+ }
+ this.subscriptions = [];
document.removeEventListener(
'visibilitychange',
this.handleVisibilityChange
diff --git a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.ts b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.ts
index 5ca7543..c2c3131 100644
--- a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.ts
+++ b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.ts
@@ -14,6 +14,7 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
+import {Subscription} from 'rxjs';
import '../../../styles/gr-a11y-styles';
import '../../../styles/shared-styles';
import '../../diff/gr-diff-cursor/gr-diff-cursor';
@@ -87,8 +88,6 @@
sizeBarInChangeTable$,
} from '../../../services/user/user-model';
import {changeComments$} from '../../../services/comments/comments-model';
-import {Subject} from 'rxjs';
-import {takeUntil} from 'rxjs/operators';
import {listen} from '../../../services/shortcuts/shortcuts-service';
import {diffViewMode$} from '../../../services/browser/browser-model';
@@ -321,7 +320,7 @@
private readonly userService = getAppContext().userService;
- disconnected$ = new Subject();
+ private subscriptions: Subscription[] = [];
/** Called in disconnectedCallback. */
private cleanups: (() => void)[] = [];
@@ -377,24 +376,24 @@
override connectedCallback() {
super.connectedCallback();
- changeComments$
- .pipe(takeUntil(this.disconnected$))
- .subscribe(changeComments => {
+ this.subscriptions.push(
+ changeComments$.subscribe(changeComments => {
this.changeComments = changeComments;
- });
- diffViewMode$
- .pipe(takeUntil(this.disconnected$))
- .subscribe(diffView => (this.diffViewMode = diffView));
- diffPreferences$
- .pipe(takeUntil(this.disconnected$))
- .subscribe(diffPreferences => {
+ })
+ );
+ this.subscriptions.push(
+ diffViewMode$.subscribe(diffView => (this.diffViewMode = diffView))
+ );
+ this.subscriptions.push(
+ diffPreferences$.subscribe(diffPreferences => {
this.diffPrefs = diffPreferences;
- });
- sizeBarInChangeTable$
- .pipe(takeUntil(this.disconnected$))
- .subscribe(sizeBarInChangeTable => {
+ })
+ );
+ this.subscriptions.push(
+ sizeBarInChangeTable$.subscribe(sizeBarInChangeTable => {
this._showSizeBars = sizeBarInChangeTable;
- });
+ })
+ );
getPluginLoader()
.awaitPluginsLoaded()
@@ -447,7 +446,10 @@
}
override disconnectedCallback() {
- this.disconnected$.next();
+ for (const s of this.subscriptions) {
+ s.unsubscribe();
+ }
+ this.subscriptions = [];
this.diffCursor.dispose();
this.fileCursor.unsetCursor();
this._cancelDiffs();
@@ -1363,8 +1365,6 @@
diffElements: GrDiffHost[],
initialCount: number
) {
- let iter = 0;
-
for (const file of files) {
const path = file.path;
const diffElem = this._findDiffByPath(path, diffElements);
@@ -1377,8 +1377,6 @@
const path = file.path;
this._cancelForEachDiff = cancel;
- iter++;
- console.info('Expanding diff', iter, 'of', initialCount, ':', path);
const diffElem = this._findDiffByPath(path, diffElements);
if (!diffElem) {
this.reporting.error(
@@ -1397,7 +1395,6 @@
return Promise.all(promises);
}).then(() => {
this._cancelForEachDiff = undefined;
- console.info('Finished expanding', initialCount, 'diff(s)');
this.reporting.timeEndWithAverage(
Timing.FILE_EXPAND_ALL,
Timing.FILE_EXPAND_ALL_AVG,
diff --git a/polygerrit-ui/app/elements/change/gr-messages-list/gr-messages-list.ts b/polygerrit-ui/app/elements/change/gr-messages-list/gr-messages-list.ts
index b3c5f8c..bd51aab 100644
--- a/polygerrit-ui/app/elements/change/gr-messages-list/gr-messages-list.ts
+++ b/polygerrit-ui/app/elements/change/gr-messages-list/gr-messages-list.ts
@@ -14,6 +14,7 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
+import {Subscription} from 'rxjs';
import '@polymer/paper-toggle-button/paper-toggle-button';
import '../../shared/gr-button/gr-button';
import '../../shared/gr-icons/gr-icons';
@@ -50,9 +51,7 @@
FormattedReviewerUpdateInfo,
ParsedChangeInfo,
} from '../../../types/types';
-import {Subject} from 'rxjs';
import {threads$} from '../../../services/comments/comments-model';
-import {takeUntil} from 'rxjs/operators';
import {
change$,
changeNum$,
@@ -268,29 +267,42 @@
private readonly shortcuts = getAppContext().shortcutsService;
- private readonly disconnected$ = new Subject();
+ private subscriptions: Subscription[] = [];
override connectedCallback() {
super.connectedCallback();
- threads$.pipe(takeUntil(this.disconnected$)).subscribe(x => {
- this.commentThreads = x;
- });
- change$.pipe(takeUntil(this.disconnected$)).subscribe(x => {
- this.change = x;
- });
- loggedIn$.pipe(takeUntil(this.disconnected$)).subscribe(x => {
- this.showReplyButtons = x;
- });
- repo$.pipe(takeUntil(this.disconnected$)).subscribe(x => {
- this.projectName = x;
- });
- changeNum$.pipe(takeUntil(this.disconnected$)).subscribe(x => {
- this.changeNum = x;
- });
+ this.subscriptions.push(
+ threads$.subscribe(x => {
+ this.commentThreads = x;
+ })
+ );
+ this.subscriptions.push(
+ change$.subscribe(x => {
+ this.change = x;
+ })
+ );
+ this.subscriptions.push(
+ loggedIn$.subscribe(x => {
+ this.showReplyButtons = x;
+ })
+ );
+ this.subscriptions.push(
+ repo$.subscribe(x => {
+ this.projectName = x;
+ })
+ );
+ this.subscriptions.push(
+ changeNum$.subscribe(x => {
+ this.changeNum = x;
+ })
+ );
}
override disconnectedCallback() {
- this.disconnected$.next();
+ for (const s of this.subscriptions) {
+ s.unsubscribe();
+ }
+ this.subscriptions = [];
super.disconnectedCallback();
}
diff --git a/polygerrit-ui/app/elements/core/gr-main-header/gr-main-header.ts b/polygerrit-ui/app/elements/core/gr-main-header/gr-main-header.ts
index 806998aa..29c8eca 100644
--- a/polygerrit-ui/app/elements/core/gr-main-header/gr-main-header.ts
+++ b/polygerrit-ui/app/elements/core/gr-main-header/gr-main-header.ts
@@ -14,6 +14,7 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
+import {Subscription} from 'rxjs';
import '../../plugins/gr-endpoint-decorator/gr-endpoint-decorator';
import '../../shared/gr-dropdown/gr-dropdown';
import '../../shared/gr-icons/gr-icons';
@@ -35,9 +36,7 @@
import {AuthType} from '../../../constants/constants';
import {DropdownLink} from '../../shared/gr-dropdown/gr-dropdown';
import {getAppContext} from '../../../services/app-context';
-import {Subject} from 'rxjs';
import {serverConfig$} from '../../../services/config/config-model';
-import {takeUntil} from 'rxjs/operators';
import {myTopMenuItems$} from '../../../services/user/user-model';
import {assertIsDefined} from '../../../utils/common-util';
@@ -161,7 +160,7 @@
private readonly userService = getAppContext().userService;
- private readonly disconnected$ = new Subject();
+ private subscriptions: Subscription[] = [];
override ready() {
super.ready();
@@ -177,22 +176,28 @@
super.connectedCallback();
this._loadAccount();
- myTopMenuItems$.pipe(takeUntil(this.disconnected$)).subscribe(items => {
- this._userLinks = items.map(this._createHeaderLink);
- });
-
- serverConfig$.pipe(takeUntil(this.disconnected$)).subscribe(config => {
- if (!config) return;
- this._retrieveFeedbackURL(config);
- this._retrieveRegisterURL(config);
- getDocsBaseUrl(config, this.restApiService).then(docBaseUrl => {
- this._docBaseUrl = docBaseUrl;
- });
- });
+ this.subscriptions.push(
+ myTopMenuItems$.subscribe(items => {
+ this._userLinks = items.map(this._createHeaderLink);
+ })
+ );
+ this.subscriptions.push(
+ serverConfig$.subscribe(config => {
+ if (!config) return;
+ this._retrieveFeedbackURL(config);
+ this._retrieveRegisterURL(config);
+ getDocsBaseUrl(config, this.restApiService).then(docBaseUrl => {
+ this._docBaseUrl = docBaseUrl;
+ });
+ })
+ );
}
override disconnectedCallback() {
- this.disconnected$.next();
+ for (const s of this.subscriptions) {
+ s.unsubscribe();
+ }
+ this.subscriptions = [];
super.disconnectedCallback();
}
diff --git a/polygerrit-ui/app/elements/diff/gr-context-controls/gr-context-controls.ts b/polygerrit-ui/app/elements/diff/gr-context-controls/gr-context-controls.ts
index af9c9d4..9673826 100644
--- a/polygerrit-ui/app/elements/diff/gr-context-controls/gr-context-controls.ts
+++ b/polygerrit-ui/app/elements/diff/gr-context-controls/gr-context-controls.ts
@@ -24,7 +24,7 @@
import '@polymer/paper-listbox/paper-listbox';
import '@polymer/paper-tooltip/paper-tooltip';
import {of, EMPTY, Subject} from 'rxjs';
-import {switchMap, delay, takeUntil} from 'rxjs/operators';
+import {switchMap, delay} from 'rxjs/operators';
import '../../shared/gr-button/gr-button';
import {pluralize} from '../../../utils/string-util';
@@ -33,6 +33,7 @@
import {assertIsDefined} from '../../../utils/common-util';
import {css, html, LitElement, TemplateResult} from 'lit';
import {customElement, property} from 'lit/decorators';
+import {subscribe} from '../../lit/subscription-controller';
import {
ContextButtonType,
@@ -98,8 +99,6 @@
linesToExpand: number;
}>();
- private disconnected$ = new Subject();
-
static override styles = css`
:host {
display: flex;
@@ -208,10 +207,6 @@
this.setupButtonHoverHandler();
}
- override disconnectedCallback() {
- this.disconnected$.next();
- }
-
private showBoth() {
return this.showConfig === 'both';
}
@@ -224,9 +219,10 @@
return this.showBoth() || this.showConfig === 'below';
}
- setupButtonHoverHandler() {
- this.expandButtonsHover
- .pipe(
+ private setupButtonHoverHandler() {
+ subscribe(
+ this,
+ this.expandButtonsHover.pipe(
switchMap(e => {
if (e.eventType === 'leave') {
// cancel any previous delay
@@ -234,15 +230,15 @@
return EMPTY;
}
return of(e).pipe(delay(500));
- }),
- takeUntil(this.disconnected$)
- )
- .subscribe(({buttonType, linesToExpand}) => {
+ })
+ ),
+ ({buttonType, linesToExpand}) => {
fire(this, 'diff-context-button-hovered', {
buttonType,
linesToExpand,
});
- });
+ }
+ );
}
private numLines() {
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.ts b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.ts
index d89ad55..8c05640 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.ts
@@ -85,10 +85,9 @@
import {Timing} from '../../../constants/reporting';
import {changeComments$} from '../../../services/comments/comments-model';
import {ChangeComments} from '../gr-comment-api/gr-comment-api';
-import {Subject} from 'rxjs';
+import {Subscription} from 'rxjs';
import {DisplayLine, RenderPreferences} from '../../../api/diff';
import {diffViewMode$} from '../../../services/browser/browser-model';
-import {takeUntil} from 'rxjs/operators';
const EMPTY_BLAME = 'No blame information for this diff.';
@@ -276,7 +275,7 @@
private readonly syntaxLayer = new GrSyntaxLayer();
- disconnected$ = new Subject();
+ private subscriptions: Subscription[] = [];
constructor() {
super();
@@ -309,21 +308,24 @@
override connectedCallback() {
super.connectedCallback();
- diffViewMode$
- .pipe(takeUntil(this.disconnected$))
- .subscribe(diffView => (this.viewMode = diffView));
+ this.subscriptions.push(
+ diffViewMode$.subscribe(diffView => (this.viewMode = diffView))
+ );
this._getLoggedIn().then(loggedIn => {
this._loggedIn = loggedIn;
});
- changeComments$
- .pipe(takeUntil(this.disconnected$))
- .subscribe(changeComments => {
+ this.subscriptions.push(
+ changeComments$.subscribe(changeComments => {
this.changeComments = changeComments;
- });
+ })
+ );
}
override disconnectedCallback() {
- this.disconnected$.next();
+ for (const s of this.subscriptions) {
+ s.unsubscribe();
+ }
+ this.subscriptions = [];
this.clear();
super.disconnectedCallback();
}
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-mode-selector/gr-diff-mode-selector.ts b/polygerrit-ui/app/elements/diff/gr-diff-mode-selector/gr-diff-mode-selector.ts
index cf328f1..4f6ae14 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-mode-selector/gr-diff-mode-selector.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff-mode-selector/gr-diff-mode-selector.ts
@@ -14,6 +14,7 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
+import {Subscription} from 'rxjs';
import '@polymer/iron-icon/iron-icon';
import '@polymer/iron-a11y-announcer/iron-a11y-announcer';
import '../../../styles/shared-styles';
@@ -27,8 +28,6 @@
import {getAppContext} from '../../../services/app-context';
import {fireIronAnnounce} from '../../../utils/event-util';
import {diffViewMode$} from '../../../services/browser/browser-model';
-import {Subject} from 'rxjs';
-import {takeUntil} from 'rxjs/operators';
@customElement('gr-diff-mode-selector')
export class GrDiffModeSelector extends PolymerElement {
@@ -51,7 +50,7 @@
private readonly userService = getAppContext().userService;
- disconnected$ = new Subject();
+ private subscriptions: Subscription[] = [];
constructor() {
super();
@@ -62,13 +61,17 @@
(
IronA11yAnnouncer as unknown as FixIronA11yAnnouncer
).requestAvailability();
- diffViewMode$
- .pipe(takeUntil(this.disconnected$))
- .subscribe(diffView => (this.mode = diffView));
+ this.subscriptions.push(
+ diffViewMode$.subscribe(diffView => (this.mode = diffView))
+ );
}
override disconnectedCallback() {
- this.disconnected$.next();
+ for (const s of this.subscriptions) {
+ s.unsubscribe();
+ }
+ this.subscriptions = [];
+ super.disconnectedCallback();
}
/**
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-preferences-dialog/gr-diff-preferences-dialog_test.ts b/polygerrit-ui/app/elements/diff/gr-diff-preferences-dialog/gr-diff-preferences-dialog_test.ts
index 5c62e7a..7f7f265 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-preferences-dialog/gr-diff-preferences-dialog_test.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff-preferences-dialog/gr-diff-preferences-dialog_test.ts
@@ -19,6 +19,7 @@
import './gr-diff-preferences-dialog';
import {GrDiffPreferencesDialog} from './gr-diff-preferences-dialog';
import {createDefaultDiffPrefs} from '../../../constants/constants';
+import {updateDiffPreferences} from '../../../services/user/user-model';
import * as MockInteractions from '@polymer/iron-test-helpers/mock-interactions';
const basicFixture = fixtureFromElement('gr-diff-preferences-dialog');
@@ -36,7 +37,8 @@
line_wrapping: true,
};
element.diffPrefs = originalDiffPrefs;
-
+ updateDiffPreferences(originalDiffPrefs);
+ await flush();
element.open();
await flush();
assert.isTrue(element.$.diffPreferences.$.lineWrappingInput.checked);
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.ts b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.ts
index 4a06679..393e093 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.ts
@@ -114,8 +114,8 @@
changeComments$,
commentsLoading$,
} from '../../../services/comments/comments-model';
-import {takeUntil, filter, take} from 'rxjs/operators';
-import {Subject, combineLatest} from 'rxjs';
+import {filter, take} from 'rxjs/operators';
+import {Subscription, combineLatest} from 'rxjs';
import {listen} from '../../../services/shortcuts/shortcuts-service';
import {
preferences$,
@@ -373,7 +373,7 @@
private cursor = new GrDiffCursor();
- disconnected$ = new Subject();
+ private subscriptions: Subscription[] = [];
constructor() {
super();
@@ -389,42 +389,44 @@
this._loggedIn = loggedIn;
});
- changeComments$
- .pipe(takeUntil(this.disconnected$))
- .subscribe(changeComments => {
+ this.subscriptions.push(
+ changeComments$.subscribe(changeComments => {
this._changeComments = changeComments;
- });
+ })
+ );
- preferences$.pipe(takeUntil(this.disconnected$)).subscribe(preferences => {
- this._userPrefs = preferences;
- });
- diffPreferences$
- .pipe(takeUntil(this.disconnected$))
- .subscribe(diffPreferences => {
+ this.subscriptions.push(
+ preferences$.subscribe(preferences => {
+ this._userPrefs = preferences;
+ })
+ );
+ this.subscriptions.push(
+ diffPreferences$.subscribe(diffPreferences => {
this._prefs = diffPreferences;
- });
+ })
+ );
// When user initially loads the diff view, we want to autmatically mark
// the file as reviewed if they have it enabled. We can't observe these
// properties since the method will be called anytime a property updates
// but we only want to call this on the initial load.
- combineLatest(currentPatchNum$, routerView$, diffPath$, diffPreferences$)
- .pipe(
- filter(
- ([currentPatchNum, routerView, path, diffPrefs]) =>
- !!currentPatchNum &&
- routerView === GerritView.DIFF &&
- !!path &&
- !!diffPrefs
- ),
- take(1)
- )
- .subscribe(([currentPatchNum, _routerView, path, diffPrefs]) => {
- this.setReviewedStatus(currentPatchNum!, path!, diffPrefs);
- });
- diffPath$
- .pipe(takeUntil(this.disconnected$))
- .subscribe(path => (this._path = path));
+ this.subscriptions.push(
+ combineLatest(currentPatchNum$, routerView$, diffPath$, diffPreferences$)
+ .pipe(
+ filter(
+ ([currentPatchNum, routerView, path, diffPrefs]) =>
+ !!currentPatchNum &&
+ routerView === GerritView.DIFF &&
+ !!path &&
+ !!diffPrefs
+ ),
+ take(1)
+ )
+ .subscribe(([currentPatchNum, _routerView, path, diffPrefs]) => {
+ this.setReviewedStatus(currentPatchNum!, path!, diffPrefs);
+ })
+ );
+ this.subscriptions.push(diffPath$.subscribe(path => (this._path = path)));
this.addEventListener('open-fix-preview', e => this._onOpenFixPreview(e));
this.cursor.replaceDiffs([this.$.diffHost]);
this._onRenderHandler = (_: Event) => {
@@ -440,13 +442,16 @@
}
override disconnectedCallback() {
- this.disconnected$.next();
this.cursor.dispose();
if (this._onRenderHandler) {
this.$.diffHost.removeEventListener('render', this._onRenderHandler);
}
for (const cleanup of this.cleanups) cleanup();
this.cleanups = [];
+ for (const s of this.subscriptions) {
+ s.unsubscribe();
+ }
+ this.subscriptions = [];
super.disconnectedCallback();
}
diff --git a/polygerrit-ui/app/elements/shared/gr-diff-preferences/gr-diff-preferences.ts b/polygerrit-ui/app/elements/shared/gr-diff-preferences/gr-diff-preferences.ts
index 779f7fb..4657020 100644
--- a/polygerrit-ui/app/elements/shared/gr-diff-preferences/gr-diff-preferences.ts
+++ b/polygerrit-ui/app/elements/shared/gr-diff-preferences/gr-diff-preferences.ts
@@ -14,6 +14,7 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
+import {Subscription} from 'rxjs';
import '@polymer/iron-input/iron-input';
import '../../../styles/shared-styles';
import '../gr-button/gr-button';
@@ -25,8 +26,6 @@
import {GrSelect} from '../gr-select/gr-select';
import {getAppContext} from '../../../services/app-context';
import {diffPreferences$} from '../../../services/user/user-model';
-import {takeUntil} from 'rxjs/operators';
-import {Subject} from 'rxjs';
export interface GrDiffPreferences {
$: {
@@ -59,19 +58,22 @@
private readonly userService = getAppContext().userService;
- private readonly disconnected$ = new Subject();
+ private subscriptions: Subscription[] = [];
override connectedCallback() {
super.connectedCallback();
- diffPreferences$
- .pipe(takeUntil(this.disconnected$))
- .subscribe(diffPreferences => {
+ this.subscriptions.push(
+ diffPreferences$.subscribe(diffPreferences => {
this.diffPrefs = diffPreferences;
- });
+ })
+ );
}
override disconnectedCallback() {
- this.disconnected$.next();
+ for (const s of this.subscriptions) {
+ s.unsubscribe();
+ }
+ this.subscriptions = [];
super.disconnectedCallback();
}
diff --git a/polygerrit-ui/app/elements/shared/gr-download-commands/gr-download-commands.ts b/polygerrit-ui/app/elements/shared/gr-download-commands/gr-download-commands.ts
index 3b3a854..68d8d7d 100644
--- a/polygerrit-ui/app/elements/shared/gr-download-commands/gr-download-commands.ts
+++ b/polygerrit-ui/app/elements/shared/gr-download-commands/gr-download-commands.ts
@@ -14,6 +14,7 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
+import {Subscription} from 'rxjs';
import '@polymer/paper-tabs/paper-tab';
import '@polymer/paper-tabs/paper-tabs';
import '../gr-shell-command/gr-shell-command';
@@ -27,8 +28,6 @@
import {queryAndAssert} from '../../../utils/common-util';
import {GrShellCommand} from '../gr-shell-command/gr-shell-command';
import {preferences$} from '../../../services/user/user-model';
-import {takeUntil} from 'rxjs/operators';
-import {Subject} from 'rxjs';
declare global {
interface HTMLElementEventMap {
@@ -76,23 +75,28 @@
private readonly userService = getAppContext().userService;
- disconnected$ = new Subject();
+ private subscriptions: Subscription[] = [];
override connectedCallback() {
super.connectedCallback();
this._getLoggedIn().then(loggedIn => {
this._loggedIn = loggedIn;
});
- preferences$.pipe(takeUntil(this.disconnected$)).subscribe(prefs => {
- if (prefs?.download_scheme) {
- // Note (issue 5180): normalize the download scheme with lower-case.
- this.selectedScheme = prefs.download_scheme.toLowerCase();
- }
- });
+ this.subscriptions.push(
+ preferences$.subscribe(prefs => {
+ if (prefs?.download_scheme) {
+ // Note (issue 5180): normalize the download scheme with lower-case.
+ this.selectedScheme = prefs.download_scheme.toLowerCase();
+ }
+ })
+ );
}
override disconnectedCallback() {
- this.disconnected$.next();
+ for (const s of this.subscriptions) {
+ s.unsubscribe();
+ }
+ this.subscriptions = [];
super.disconnectedCallback();
}
diff --git a/polygerrit-ui/app/elements/shared/gr-download-commands/gr-download-commands_test.ts b/polygerrit-ui/app/elements/shared/gr-download-commands/gr-download-commands_test.ts
index ef712ac..6cbef79 100644
--- a/polygerrit-ui/app/elements/shared/gr-download-commands/gr-download-commands_test.ts
+++ b/polygerrit-ui/app/elements/shared/gr-download-commands/gr-download-commands_test.ts
@@ -114,22 +114,22 @@
});
suite('authenticated', () => {
test('loads scheme from preferences', async () => {
+ const element = basicFixture.instantiate();
+ await flush();
updatePreferences({
...createPreferences(),
download_scheme: 'repo',
});
- const element = basicFixture.instantiate();
- await flush();
assert.equal(element.selectedScheme, 'repo');
});
test('normalize scheme from preferences', async () => {
+ const element = basicFixture.instantiate();
+ await flush();
updatePreferences({
...createPreferences(),
download_scheme: 'REPO',
});
- const element = basicFixture.instantiate();
- await flush();
assert.equal(element.selectedScheme, 'repo');
});
});
diff --git a/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-gerrit.ts b/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-gerrit.ts
index 4ad801d..d76b2b7 100644
--- a/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-gerrit.ts
+++ b/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-gerrit.ts
@@ -151,6 +151,8 @@
assertIsDefined(this.restApiService, 'restApiService');
}
+ finalize() {}
+
/**
* @deprecated Use plugin.styles().css(rulesStr) instead. Please, consult
* the documentation how to replace it accordingly.
diff --git a/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-js-api-interface-element.ts b/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-js-api-interface-element.ts
index 8015505..0bd3e59 100644
--- a/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-js-api-interface-element.ts
+++ b/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-js-api-interface-element.ts
@@ -33,14 +33,17 @@
import {EventType, TargetElement} from '../../../api/plugin';
import {DiffLayer, HighlightJS, ParsedChangeInfo} from '../../../types/types';
import {MenuLink} from '../../../api/admin';
+import {Finalizable} from '../../../services/registry';
import {ReportingService} from '../../../services/gr-reporting/gr-reporting';
const elements: {[key: string]: HTMLElement} = {};
const eventCallbacks: {[key: string]: EventCallback[]} = {};
-export class GrJsApiInterface implements JsApiService {
+export class GrJsApiInterface implements JsApiService, Finalizable {
constructor(readonly reporting: ReportingService) {}
+ finalize() {}
+
// eslint-disable-next-line @typescript-eslint/no-explicit-any
handleEvent(type: EventType, detail: any) {
getPluginLoader()
diff --git a/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-js-api-types.ts b/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-js-api-types.ts
index 9644ef3..7e6a0c7 100644
--- a/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-js-api-types.ts
+++ b/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-js-api-types.ts
@@ -21,6 +21,7 @@
ReviewInput,
RevisionInfo,
} from '../../../types/common';
+import {Finalizable} from '../../../services/registry';
import {EventType, TargetElement} from '../../../api/plugin';
import {DiffLayer, ParsedChangeInfo} from '../../../types/types';
import {GrAnnotationActionsInterface} from './gr-annotation-actions-js-api';
@@ -40,7 +41,7 @@
// eslint-disable-next-line @typescript-eslint/no-explicit-any
export type EventCallback = (...args: any[]) => any;
-export interface JsApiService {
+export interface JsApiService extends Finalizable {
getElement(key: TargetElement): HTMLElement;
addEventCallback(eventName: EventType, callback: EventCallback): void;
modifyRevertSubmissionMsg(
diff --git a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.ts b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.ts
index 5e8e733..9b292e9 100644
--- a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.ts
+++ b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.ts
@@ -33,6 +33,7 @@
import {parseDate} from '../../../utils/date-util';
import {getBaseUrl} from '../../../utils/url-util';
import {getAppContext} from '../../../services/app-context';
+import {Finalizable} from '../../../services/registry';
import {getParentIndex, isMergeParent} from '../../../utils/patch-set-util';
import {
ListChangesOption,
@@ -275,7 +276,7 @@
@customElement('gr-rest-api-interface')
export class GrRestApiInterface
extends PolymerElement
- implements RestApiService
+ implements RestApiService, Finalizable
{
readonly _cache = siteBasedCache; // Shared across instances.
@@ -308,6 +309,8 @@
);
}
+ finalize() {}
+
_fetchSharedCacheURL(req: FetchJSONRequest): Promise<ParsedJSON | undefined> {
// Cache is shared across instances
return this._restApiHelper.fetchCacheURL(req);
diff --git a/polygerrit-ui/app/embed/gr-diff-app-context-init.ts b/polygerrit-ui/app/embed/gr-diff-app-context-init.ts
index 21a9de89..229bce3 100644
--- a/polygerrit-ui/app/embed/gr-diff-app-context-init.ts
+++ b/polygerrit-ui/app/embed/gr-diff-app-context-init.ts
@@ -25,6 +25,8 @@
return false;
}
+ finalize() {}
+
/**
* @returns array of all enabled experiments.
*/
@@ -48,6 +50,8 @@
setup() {}
+ finalize() {}
+
fetch() {
const blob = new Blob();
const init = {status: 200, statusText: 'Ack'};
diff --git a/polygerrit-ui/app/services/app-context-init.ts b/polygerrit-ui/app/services/app-context-init.ts
index 74f4ee5..32a4560 100644
--- a/polygerrit-ui/app/services/app-context-init.ts
+++ b/polygerrit-ui/app/services/app-context-init.ts
@@ -15,7 +15,7 @@
* limitations under the License.
*/
import {AppContext} from './app-context';
-import {create, Registry} from './registry';
+import {create, Finalizable, Registry} from './registry';
import {FlagsServiceImplementation} from './flags/flags_impl';
import {GrReporting} from './gr-reporting/gr-reporting_impl';
import {EventEmitter} from './gr-event-interface/gr-event-interface_impl';
@@ -32,16 +32,10 @@
import {BrowserService} from './browser/browser-service';
import {assertIsDefined} from '../utils/common-util';
-type ServiceName = keyof AppContext;
-// NOTE: This global table is a stopgap solution until services know how to
-// properly clean up after themselves.
-// eslint-disable-next-line @typescript-eslint/no-explicit-any
-const initializedServices: Map<ServiceName, any> = new Map<ServiceName, any>();
-
/**
* The AppContext lazy initializator for all services
*/
-export function createAppContext(): AppContext {
+export function createAppContext(): AppContext & Finalizable {
const appRegistry: Registry<AppContext> = {
flagsService: (_ctx: Partial<AppContext>) =>
new FlagsServiceImplementation(),
@@ -90,5 +84,5 @@
},
browserService: (_ctx: Partial<AppContext>) => new BrowserService(),
};
- return create<AppContext>(appRegistry, initializedServices) as AppContext;
+ return create<AppContext>(appRegistry);
}
diff --git a/polygerrit-ui/app/services/app-context-init_test.ts b/polygerrit-ui/app/services/app-context-init_test.ts
index 16344a1..efe6aac 100644
--- a/polygerrit-ui/app/services/app-context-init_test.ts
+++ b/polygerrit-ui/app/services/app-context-init_test.ts
@@ -18,12 +18,18 @@
import '../test/common-test-setup-karma.js';
import {AppContext} from './app-context.js';
import {createAppContext} from './app-context-init.js';
+import {Finalizable} from './registry';
+
suite('app context initializer tests', () => {
- let appContext: Partial<AppContext>;
+ let appContext: AppContext & Finalizable;
setup(() => {
appContext = createAppContext();
});
+ teardown(() => {
+ appContext.finalize();
+ });
+
test('all services initialized and are singletons', () => {
Object.keys(appContext).forEach(serviceName => {
const service = appContext[serviceName as keyof AppContext];
diff --git a/polygerrit-ui/app/services/app-context.ts b/polygerrit-ui/app/services/app-context.ts
index 71d0147..49d5bcc 100644
--- a/polygerrit-ui/app/services/app-context.ts
+++ b/polygerrit-ui/app/services/app-context.ts
@@ -14,6 +14,7 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
+import {Finalizable} from './registry';
import {FlagsService} from './flags/flags';
import {EventEmitterService} from './gr-event-interface/gr-event-interface';
import {ReportingService} from './gr-reporting/gr-reporting';
@@ -55,12 +56,14 @@
* It is guaranteed that all fields in appContext are always initialized
* (except for shared gr-diff)
*/
-let appContext: AppContext = {} as AppContext;
+let appContext: (AppContext & Finalizable) | undefined = undefined;
-export function injectAppContext(ctx: AppContext) {
+export function injectAppContext(ctx: AppContext & Finalizable) {
+ appContext?.finalize();
appContext = ctx;
}
export function getAppContext() {
+ if (!appContext) throw new Error('App context has not been injected');
return appContext;
}
diff --git a/polygerrit-ui/app/services/browser/browser-service.ts b/polygerrit-ui/app/services/browser/browser-service.ts
index d98f8f7..b80c4df 100644
--- a/polygerrit-ui/app/services/browser/browser-service.ts
+++ b/polygerrit-ui/app/services/browser/browser-service.ts
@@ -1,5 +1,3 @@
-import {updateStateScreenWidth} from './browser-model';
-
/**
* @license
* Copyright (C) 2021 The Android Open Source Project
@@ -17,7 +15,10 @@
* limitations under the License.
*/
-export class BrowserService {
+import {updateStateScreenWidth} from './browser-model';
+import {Finalizable} from '../registry';
+
+export class BrowserService implements Finalizable {
/* Observer the screen width so that the app can react to changes to it */
observeWidth() {
return new ResizeObserver(entries => {
@@ -26,4 +27,6 @@
});
});
}
+
+ finalize() {}
}
diff --git a/polygerrit-ui/app/services/change/change-service.ts b/polygerrit-ui/app/services/change/change-service.ts
index 7a6bdd6..7a2d2f4 100644
--- a/polygerrit-ui/app/services/change/change-service.ts
+++ b/polygerrit-ui/app/services/change/change-service.ts
@@ -14,6 +14,7 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
+import {Subscription} from 'rxjs';
import {routerChangeNum$} from '../router/router-model';
import {change$, updateStateChange, updateStatePath} from './change-model';
import {ParsedChangeInfo} from '../../types/types';
@@ -23,20 +24,34 @@
computeLatestPatchNum,
} from '../../utils/patch-set-util';
import {RestApiService} from '../gr-rest-api/gr-rest-api';
+import {Finalizable} from '../registry';
-export class ChangeService {
+export class ChangeService implements Finalizable {
private change?: ParsedChangeInfo;
+ private readonly subscriptions: Subscription[] = [];
+
constructor(readonly restApiService: RestApiService) {
// TODO: In the future we will want to make restApiService.getChangeDetail()
// calls from a switchMap() here. For now just make sure to invalidate the
// change when no changeNum is set.
- routerChangeNum$.subscribe(changeNum => {
- if (!changeNum) updateStateChange(undefined);
- });
- change$.subscribe(change => {
- this.change = change;
- });
+ this.subscriptions.push(
+ routerChangeNum$.subscribe(changeNum => {
+ if (!changeNum) updateStateChange(undefined);
+ })
+ );
+ this.subscriptions.push(
+ change$.subscribe(change => {
+ this.change = change;
+ })
+ );
+ }
+
+ finalize() {
+ for (const s of this.subscriptions) {
+ s.unsubscribe();
+ }
+ this.subscriptions.splice(0, this.subscriptions.length);
}
/**
diff --git a/polygerrit-ui/app/services/checks/checks-service.ts b/polygerrit-ui/app/services/checks/checks-service.ts
index 5ebc13c..111036c 100644
--- a/polygerrit-ui/app/services/checks/checks-service.ts
+++ b/polygerrit-ui/app/services/checks/checks-service.ts
@@ -15,6 +15,16 @@
* limitations under the License.
*/
import {
+ BehaviorSubject,
+ combineLatest,
+ from,
+ Observable,
+ of,
+ Subject,
+ Subscription,
+ timer,
+} from 'rxjs';
+import {
catchError,
filter,
switchMap,
@@ -46,16 +56,8 @@
updateStateSetResults,
updateStateUpdateResult,
} from './checks-model';
-import {
- BehaviorSubject,
- combineLatest,
- from,
- Observable,
- of,
- Subject,
- timer,
-} from 'rxjs';
import {ChangeInfo, NumericChangeId, PatchSetNumber} from '../../types/common';
+import {Finalizable} from '../registry';
import {getCurrentRevision} from '../../utils/change-util';
import {getShaByPatchNum} from '../../utils/patch-set-util';
import {assertIsDefined} from '../../utils/common-util';
@@ -64,7 +66,7 @@
import {Execution} from '../../constants/reporting';
import {fireAlert, fireEvent} from '../../utils/event-util';
-export class ChecksService {
+export class ChecksService implements Finalizable {
private readonly providers: {[name: string]: ChecksProvider} = {};
private readonly reloadSubjects: {[name: string]: Subject<void>} = {};
@@ -77,29 +79,54 @@
private readonly documentVisibilityChange$ = new BehaviorSubject(undefined);
+ private readonly reloadListener: () => void;
+
+ private readonly subscriptions: Subscription[] = [];
+
+ private readonly visibilityChangeListener: () => void;
+
constructor(readonly reporting: ReportingService) {
- changeNum$.subscribe(x => (this.changeNum = x));
- checkToPluginMap$.subscribe(map => {
- this.checkToPluginMap = map;
- });
- combineLatest([routerPatchNum$, latestPatchNum$]).subscribe(
- ([routerPs, latestPs]) => {
- this.latestPatchNum = latestPs;
- if (latestPs === undefined) {
- this.setPatchset(undefined);
- } else if (typeof routerPs === 'number') {
- this.setPatchset(routerPs);
- } else {
- this.setPatchset(latestPs);
- }
- }
+ this.subscriptions.push(changeNum$.subscribe(x => (this.changeNum = x)));
+ this.subscriptions.push(
+ checkToPluginMap$.subscribe(map => {
+ this.checkToPluginMap = map;
+ })
);
- document.addEventListener('visibilitychange', () => {
+ this.subscriptions.push(
+ combineLatest([routerPatchNum$, latestPatchNum$]).subscribe(
+ ([routerPs, latestPs]) => {
+ this.latestPatchNum = latestPs;
+ if (latestPs === undefined) {
+ this.setPatchset(undefined);
+ } else if (typeof routerPs === 'number') {
+ this.setPatchset(routerPs);
+ } else {
+ this.setPatchset(latestPs);
+ }
+ }
+ )
+ );
+ this.visibilityChangeListener = () => {
this.documentVisibilityChange$.next(undefined);
- });
- document.addEventListener('reload', () => {
- this.reloadAll();
- });
+ };
+ document.addEventListener(
+ 'visibilitychange',
+ this.visibilityChangeListener
+ );
+ this.reloadListener = () => this.reloadAll();
+ document.addEventListener('reload', this.reloadListener);
+ }
+
+ finalize() {
+ document.removeEventListener('reload', this.reloadListener);
+ document.removeEventListener(
+ 'visibilitychange',
+ this.visibilityChangeListener
+ );
+ for (const s of this.subscriptions) {
+ s.unsubscribe();
+ }
+ this.subscriptions.splice(0, this.subscriptions.length);
}
setPatchset(num?: PatchSetNumber) {
@@ -188,82 +215,87 @@
// 2. Specific reload requests.
// 3. Regular polling starting with an initial fetch right now.
// 4. A hidden Gerrit tab becoming visible.
- combineLatest([
- changeNum$,
- patchset === ChecksPatchset.LATEST
- ? latestPatchNum$
- : checksSelectedPatchsetNumber$,
- this.reloadSubjects[pluginName].pipe(throttleTime(1000)),
- timer(0, pollIntervalMs),
- this.documentVisibilityChange$,
- ])
- .pipe(
- takeWhile(_ => !!this.providers[pluginName]),
- filter(_ => document.visibilityState !== 'hidden'),
- withLatestFrom(change$),
- switchMap(
- ([[changeNum, patchNum], change]): Observable<FetchResponse> => {
- if (!change || !changeNum || !patchNum) return of(this.empty());
- if (typeof patchNum !== 'number') return of(this.empty());
- assertIsDefined(change.revisions, 'change.revisions');
- const patchsetSha = getShaByPatchNum(change.revisions, patchNum);
- // Sometimes patchNum is updated earlier than change, so change
- // revisions don't have patchNum yet
- if (!patchsetSha) return of(this.empty());
- const data: ChangeData = {
- changeNumber: changeNum,
- patchsetNumber: patchNum,
- patchsetSha,
- repo: change.project,
- commitMessage: getCurrentRevision(change)?.commit?.message,
- changeInfo: change as ChangeInfo,
- };
- return this.fetchResults(pluginName, data, patchset);
+ this.subscriptions.push(
+ combineLatest([
+ changeNum$,
+ patchset === ChecksPatchset.LATEST
+ ? latestPatchNum$
+ : checksSelectedPatchsetNumber$,
+ this.reloadSubjects[pluginName].pipe(throttleTime(1000)),
+ timer(0, pollIntervalMs),
+ this.documentVisibilityChange$,
+ ])
+ .pipe(
+ takeWhile(_ => !!this.providers[pluginName]),
+ filter(_ => document.visibilityState !== 'hidden'),
+ withLatestFrom(change$),
+ switchMap(
+ ([[changeNum, patchNum], change]): Observable<FetchResponse> => {
+ if (!change || !changeNum || !patchNum) return of(this.empty());
+ if (typeof patchNum !== 'number') return of(this.empty());
+ assertIsDefined(change.revisions, 'change.revisions');
+ const patchsetSha = getShaByPatchNum(change.revisions, patchNum);
+ // Sometimes patchNum is updated earlier than change, so change
+ // revisions don't have patchNum yet
+ if (!patchsetSha) return of(this.empty());
+ const data: ChangeData = {
+ changeNumber: changeNum,
+ patchsetNumber: patchNum,
+ patchsetSha,
+ repo: change.project,
+ commitMessage: getCurrentRevision(change)?.commit?.message,
+ changeInfo: change as ChangeInfo,
+ };
+ return this.fetchResults(pluginName, data, patchset);
+ }
+ ),
+ catchError(e => {
+ // This should not happen and is really severe, because it means that
+ // the Observable has terminated and we won't recover from that. No
+ // further attempts to fetch results for this plugin will be made.
+ this.reporting.error(e, `checks-service crash for ${pluginName}`);
+ return of(this.createErrorResponse(pluginName, e));
+ })
+ )
+ .subscribe(response => {
+ switch (response.responseCode) {
+ case ResponseCode.ERROR: {
+ const message = response.errorMessage ?? '-';
+ this.reporting.reportExecution(Execution.CHECKS_API_ERROR, {
+ plugin: pluginName,
+ message,
+ });
+ updateStateSetError(pluginName, message, patchset);
+ break;
+ }
+ case ResponseCode.NOT_LOGGED_IN: {
+ assertIsDefined(response.loginCallback, 'loginCallback');
+ this.reporting.reportExecution(
+ Execution.CHECKS_API_NOT_LOGGED_IN,
+ {
+ plugin: pluginName,
+ }
+ );
+ updateStateSetNotLoggedIn(
+ pluginName,
+ response.loginCallback,
+ patchset
+ );
+ break;
+ }
+ case ResponseCode.OK: {
+ updateStateSetResults(
+ pluginName,
+ response.runs ?? [],
+ response.actions ?? [],
+ response.links ?? [],
+ patchset
+ );
+ break;
+ }
}
- ),
- catchError(e => {
- // This should not happen and is really severe, because it means that
- // the Observable has terminated and we won't recover from that. No
- // further attempts to fetch results for this plugin will be made.
- this.reporting.error(e, `checks-service crash for ${pluginName}`);
- return of(this.createErrorResponse(pluginName, e));
})
- )
- .subscribe(response => {
- switch (response.responseCode) {
- case ResponseCode.ERROR: {
- const message = response.errorMessage ?? '-';
- this.reporting.reportExecution(Execution.CHECKS_API_ERROR, {
- plugin: pluginName,
- message,
- });
- updateStateSetError(pluginName, message, patchset);
- break;
- }
- case ResponseCode.NOT_LOGGED_IN: {
- assertIsDefined(response.loginCallback, 'loginCallback');
- this.reporting.reportExecution(Execution.CHECKS_API_NOT_LOGGED_IN, {
- plugin: pluginName,
- });
- updateStateSetNotLoggedIn(
- pluginName,
- response.loginCallback,
- patchset
- );
- break;
- }
- case ResponseCode.OK: {
- updateStateSetResults(
- pluginName,
- response.runs ?? [],
- response.actions ?? [],
- response.links ?? [],
- patchset
- );
- break;
- }
- }
- });
+ );
}
private empty(): FetchResponse {
diff --git a/polygerrit-ui/app/services/comments/comments-service.ts b/polygerrit-ui/app/services/comments/comments-service.ts
index b9dc17f..c888cd5 100644
--- a/polygerrit-ui/app/services/comments/comments-service.ts
+++ b/polygerrit-ui/app/services/comments/comments-service.ts
@@ -14,7 +14,7 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
-
+import {combineLatest, Subscription} from 'rxjs';
import {NumericChangeId, PatchSetNum, RevisionId} from '../../types/common';
import {DraftInfo, UIDraft} from '../../utils/comment-util';
import {fireAlert} from '../../utils/event-util';
@@ -34,36 +34,56 @@
updateStateReset,
} from './comments-model';
import {changeNum$, currentPatchNum$} from '../change/change-model';
-import {combineLatest} from 'rxjs';
-import {routerChangeNum$} from '../router/router-model';
-export class CommentsService {
+import {routerChangeNum$} from '../router/router-model';
+import {Finalizable} from '../registry';
+
+export class CommentsService implements Finalizable {
private discardedDrafts?: UIDraft[] = [];
private changeNum?: NumericChangeId;
private patchNum?: PatchSetNum;
+ private readonly reloadListener: () => void;
+
+ private readonly subscriptions: Subscription[] = [];
+
constructor(readonly restApiService: RestApiService) {
- discardedDrafts$.subscribe(
- discardedDrafts => (this.discardedDrafts = discardedDrafts)
+ this.subscriptions.push(
+ discardedDrafts$.subscribe(
+ discardedDrafts => (this.discardedDrafts = discardedDrafts)
+ )
);
- routerChangeNum$.subscribe(changeNum => {
- this.changeNum = changeNum;
- updateStateReset();
- this.reloadAllComments();
- });
- combineLatest([changeNum$, currentPatchNum$]).subscribe(
- ([changeNum, patchNum]) => {
+ this.subscriptions.push(
+ routerChangeNum$.subscribe(changeNum => {
this.changeNum = changeNum;
- this.patchNum = patchNum;
- this.reloadAllPortedComments();
- }
+ updateStateReset();
+ this.reloadAllComments();
+ })
);
- document.addEventListener('reload', () => {
+ this.subscriptions.push(
+ combineLatest([changeNum$, currentPatchNum$]).subscribe(
+ ([changeNum, patchNum]) => {
+ this.changeNum = changeNum;
+ this.patchNum = patchNum;
+ this.reloadAllPortedComments();
+ }
+ )
+ );
+ this.reloadListener = () => {
this.reloadAllComments();
this.reloadAllPortedComments();
- });
+ };
+ document.addEventListener('reload', this.reloadListener);
+ }
+
+ finalize() {
+ document.removeEventListener('reload', this.reloadListener!);
+ for (const s of this.subscriptions) {
+ s.unsubscribe();
+ }
+ this.subscriptions.splice(0, this.subscriptions.length);
}
// Note that this does *not* reload ported comments.
diff --git a/polygerrit-ui/app/services/comments/comments-service_test.ts b/polygerrit-ui/app/services/comments/comments-service_test.ts
index d0309a8..5fe859f 100644
--- a/polygerrit-ui/app/services/comments/comments-service_test.ts
+++ b/polygerrit-ui/app/services/comments/comments-service_test.ts
@@ -14,7 +14,7 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
-
+import {Subscription} from 'rxjs';
import '../../test/common-test-setup-karma';
import {
createComment,
@@ -33,6 +33,15 @@
import {PathToCommentsInfoMap} from '../../types/common';
suite('change service tests', () => {
+ let subscriptions: Subscription[] = [];
+
+ teardown(() => {
+ for (const s of subscriptions) {
+ s.unsubscribe();
+ }
+ subscriptions = [];
+ });
+
test('loads comments', async () => {
new CommentsService(getAppContext().restApiService);
const diffCommentsSpy = stubRestApi('getDiffComments').returns(
@@ -51,9 +60,11 @@
Promise.resolve({})
);
let comments: PathToCommentsInfoMap = {};
- comments$.subscribe(c => (comments = c ?? {}));
+ subscriptions.push(comments$.subscribe(c => (comments = c ?? {})));
let portedComments: PathToCommentsInfoMap = {};
- portedComments$.subscribe(c => (portedComments = c ?? {}));
+ subscriptions.push(
+ portedComments$.subscribe(c => (portedComments = c ?? {}))
+ );
updateRouterState(GerritView.CHANGE, TEST_NUMERIC_CHANGE_ID);
updateStateChange(createParsedChange());
diff --git a/polygerrit-ui/app/services/config/config-service.ts b/polygerrit-ui/app/services/config/config-service.ts
index 6d01e27..667f347 100644
--- a/polygerrit-ui/app/services/config/config-service.ts
+++ b/polygerrit-ui/app/services/config/config-service.ts
@@ -18,23 +18,37 @@
import {repo$} from '../change/change-model';
import {switchMap} from 'rxjs/operators';
import {ConfigInfo, RepoName, ServerInfo} from '../../types/common';
-import {from, of} from 'rxjs';
+import {from, of, Subscription} from 'rxjs';
import {RestApiService} from '../gr-rest-api/gr-rest-api';
+import {Finalizable} from '../registry';
-export class ConfigService {
+export class ConfigService implements Finalizable {
+ private readonly subscriptions: Subscription[] = [];
+
constructor(readonly restApiService: RestApiService) {
- from(this.restApiService.getConfig()).subscribe((config?: ServerInfo) => {
- updateServerConfig(config);
- });
- repo$
- .pipe(
- switchMap((repo?: RepoName) => {
- if (repo === undefined) return of(undefined);
- return from(this.restApiService.getProjectConfig(repo));
+ this.subscriptions.push(
+ from(this.restApiService.getConfig()).subscribe((config?: ServerInfo) => {
+ updateServerConfig(config);
+ })
+ );
+ this.subscriptions.push(
+ repo$
+ .pipe(
+ switchMap((repo?: RepoName) => {
+ if (repo === undefined) return of(undefined);
+ return from(this.restApiService.getProjectConfig(repo));
+ })
+ )
+ .subscribe((repoConfig?: ConfigInfo) => {
+ updateRepoConfig(repoConfig);
})
- )
- .subscribe((repoConfig?: ConfigInfo) => {
- updateRepoConfig(repoConfig);
- });
+ );
+ }
+
+ finalize() {
+ for (const s of this.subscriptions) {
+ s.unsubscribe();
+ }
+ this.subscriptions.splice(0, this.subscriptions.length);
}
}
diff --git a/polygerrit-ui/app/services/flags/flags.ts b/polygerrit-ui/app/services/flags/flags.ts
index 318ea35..b688e8f 100644
--- a/polygerrit-ui/app/services/flags/flags.ts
+++ b/polygerrit-ui/app/services/flags/flags.ts
@@ -15,7 +15,9 @@
* limitations under the License.
*/
-export interface FlagsService {
+import {Finalizable} from '../registry';
+
+export interface FlagsService extends Finalizable {
isEnabled(experimentId: string): boolean;
enabledExperiments: string[];
}
diff --git a/polygerrit-ui/app/services/flags/flags_impl.ts b/polygerrit-ui/app/services/flags/flags_impl.ts
index 18e225b..9767d1a 100644
--- a/polygerrit-ui/app/services/flags/flags_impl.ts
+++ b/polygerrit-ui/app/services/flags/flags_impl.ts
@@ -15,6 +15,7 @@
* limitations under the License.
*/
import {FlagsService} from './flags';
+import {Finalizable} from '../registry';
declare global {
interface Window {
@@ -27,7 +28,7 @@
*
* Provides all related methods / properties regarding on feature flags.
*/
-export class FlagsServiceImplementation implements FlagsService {
+export class FlagsServiceImplementation implements FlagsService, Finalizable {
private readonly _experiments: Set<string>;
constructor() {
@@ -35,6 +36,8 @@
this._experiments = this._loadExperiments();
}
+ finalize() {}
+
isEnabled(experimentId: string): boolean {
return this._experiments.has(experimentId);
}
diff --git a/polygerrit-ui/app/services/gr-auth/gr-auth.ts b/polygerrit-ui/app/services/gr-auth/gr-auth.ts
index f7fdadf..ac63d6a 100644
--- a/polygerrit-ui/app/services/gr-auth/gr-auth.ts
+++ b/polygerrit-ui/app/services/gr-auth/gr-auth.ts
@@ -14,7 +14,7 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
-
+import {Finalizable} from '../registry';
export enum AuthType {
XSRF_TOKEN = 'xsrf_token',
ACCESS_TOKEN = 'access_token',
@@ -45,7 +45,7 @@
headers?: Headers;
}
-export interface AuthService {
+export interface AuthService extends Finalizable {
baseUrl: string;
isAuthed: boolean;
diff --git a/polygerrit-ui/app/services/gr-auth/gr-auth_impl.ts b/polygerrit-ui/app/services/gr-auth/gr-auth_impl.ts
index c254284..b740d29 100644
--- a/polygerrit-ui/app/services/gr-auth/gr-auth_impl.ts
+++ b/polygerrit-ui/app/services/gr-auth/gr-auth_impl.ts
@@ -16,6 +16,7 @@
*/
import {getBaseUrl} from '../../utils/url-util';
import {EventEmitterService} from '../gr-event-interface/gr-event-interface';
+import {Finalizable} from '../registry';
import {
AuthRequestInit,
AuthService,
@@ -44,7 +45,7 @@
/**
* Auth class.
*/
-export class Auth implements AuthService {
+export class Auth implements AuthService, Finalizable {
// TODO(dmfilippov): Remove Type and Status properties, expose AuthType and
// AuthStatus to API
static TYPE = {
@@ -88,6 +89,8 @@
return getBaseUrl();
}
+ finalize() {}
+
/**
* Returns if user is authed or not.
*/
diff --git a/polygerrit-ui/app/services/gr-auth/gr-auth_mock.ts b/polygerrit-ui/app/services/gr-auth/gr-auth_mock.ts
index 3dbb4c3..e5331f1 100644
--- a/polygerrit-ui/app/services/gr-auth/gr-auth_mock.ts
+++ b/polygerrit-ui/app/services/gr-auth/gr-auth_mock.ts
@@ -40,6 +40,8 @@
return this._status === Auth.STATUS.AUTHED;
}
+ finalize() {}
+
private _setStatus(status: AuthStatus) {
if (this._status === status) return;
if (this._status === AuthStatus.AUTHED) {
diff --git a/polygerrit-ui/app/services/gr-event-interface/gr-event-interface.ts b/polygerrit-ui/app/services/gr-event-interface/gr-event-interface.ts
index e540029..391a32b 100644
--- a/polygerrit-ui/app/services/gr-event-interface/gr-event-interface.ts
+++ b/polygerrit-ui/app/services/gr-event-interface/gr-event-interface.ts
@@ -14,12 +14,12 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
-
+import {Finalizable} from '../registry';
// eslint-disable-next-line @typescript-eslint/no-explicit-any
export type EventCallback = (...args: any) => void;
export type UnsubscribeMethod = () => void;
-export interface EventEmitterService {
+export interface EventEmitterService extends Finalizable {
/**
* Register an event listener to an event.
*/
diff --git a/polygerrit-ui/app/services/gr-event-interface/gr-event-interface_impl.ts b/polygerrit-ui/app/services/gr-event-interface/gr-event-interface_impl.ts
index d8c5d77..19d2f61 100644
--- a/polygerrit-ui/app/services/gr-event-interface/gr-event-interface_impl.ts
+++ b/polygerrit-ui/app/services/gr-event-interface/gr-event-interface_impl.ts
@@ -14,7 +14,7 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
-
+import {Finalizable} from '../registry';
import {
EventCallback,
EventEmitterService,
@@ -34,9 +34,13 @@
* }
*
*/
-export class EventEmitter implements EventEmitterService {
+export class EventEmitter implements EventEmitterService, Finalizable {
private _listenersMap = new Map<string, EventCallback[]>();
+ finalize() {
+ this.removeAllListeners();
+ }
+
/**
* Register an event listener to an event.
*/
@@ -123,7 +127,7 @@
*
* @param eventName if not provided, will remove all
*/
- removeAllListeners(eventName: string): void {
+ removeAllListeners(eventName?: string): void {
if (eventName) {
this._listenersMap.set(eventName, []);
} else {
diff --git a/polygerrit-ui/app/services/gr-reporting/gr-reporting.ts b/polygerrit-ui/app/services/gr-reporting/gr-reporting.ts
index 06f1a0c..679fefc 100644
--- a/polygerrit-ui/app/services/gr-reporting/gr-reporting.ts
+++ b/polygerrit-ui/app/services/gr-reporting/gr-reporting.ts
@@ -14,7 +14,7 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
-
+import {Finalizable} from '../registry';
import {NumericChangeId} from '../../types/common';
import {EventDetails} from '../../api/reporting';
import {PluginApi} from '../../api/plugin';
@@ -33,7 +33,7 @@
withMaximum(maximum: number): this;
}
-export interface ReportingService {
+export interface ReportingService extends Finalizable {
reporter(
type: string,
category: string,
diff --git a/polygerrit-ui/app/services/gr-reporting/gr-reporting_impl.ts b/polygerrit-ui/app/services/gr-reporting/gr-reporting_impl.ts
index 4d9b76f..7d03de5 100644
--- a/polygerrit-ui/app/services/gr-reporting/gr-reporting_impl.ts
+++ b/polygerrit-ui/app/services/gr-reporting/gr-reporting_impl.ts
@@ -20,6 +20,7 @@
import {NumericChangeId} from '../../types/common';
import {EventDetails} from '../../api/reporting';
import {PluginApi} from '../../api/plugin';
+import {Finalizable} from '../registry';
import {
Execution,
Interaction,
@@ -272,7 +273,7 @@
type PendingReportInfo = [EventInfo, boolean | undefined];
-export class GrReporting implements ReportingService {
+export class GrReporting implements ReportingService, Finalizable {
private readonly _flagsService: FlagsService;
private readonly _baselines = STARTUP_TIMERS;
@@ -323,6 +324,8 @@
);
}
+ finalize() {}
+
/**
* Reporter reports events. Events will be queued if metrics plugin is not
* yet installed.
diff --git a/polygerrit-ui/app/services/gr-reporting/gr-reporting_mock.ts b/polygerrit-ui/app/services/gr-reporting/gr-reporting_mock.ts
index 337cf2f..485402b 100644
--- a/polygerrit-ui/app/services/gr-reporting/gr-reporting_mock.ts
+++ b/polygerrit-ui/app/services/gr-reporting/gr-reporting_mock.ts
@@ -18,6 +18,7 @@
import {EventDetails} from '../../api/reporting';
import {PluginApi} from '../../api/plugin';
import {Execution, Interaction} from '../../constants/reporting';
+import {Finalizable} from '../registry';
export class MockTimer implements Timer {
end(): this {
@@ -37,7 +38,7 @@
console.info(`ReportingMock.${msg}`);
};
-export const grReportingMock: ReportingService = {
+export const grReportingMock: ReportingService & Finalizable = {
appStarted: () => {},
beforeLocationChanged: () => {},
changeDisplayed: () => {},
@@ -47,6 +48,7 @@
diffViewDisplayed: () => {},
diffViewFullyLoaded: () => {},
fileListDisplayed: () => {},
+ finalize: () => {},
getTimer: () => new MockTimer(),
locationChanged: (page: string) => {
log(`locationChanged: ${page}`);
@@ -65,20 +67,13 @@
error: () => {
log('error');
},
- reportExecution: (id: Execution, details?: EventDetails) => {
- log(`reportExecution '${id}': ${JSON.stringify(details)}`);
- },
- trackApi: (pluginApi: PluginApi, object: string, method: string) => {
- const plugin = pluginApi?.getPluginName() ?? 'unknown';
- log(`trackApi '${plugin}', ${object}, ${method}`);
- },
+ reportExecution: (_id: Execution, _details?: EventDetails) => {},
+ trackApi: (_pluginApi: PluginApi, _object: string, _method: string) => {},
reportExtension: () => {},
reportInteraction: (
- eventName: string | Interaction,
- details?: EventDetails
- ) => {
- log(`reportInteraction '${eventName}': ${JSON.stringify(details)}`);
- },
+ _eventName: string | Interaction,
+ _details?: EventDetails
+ ) => {},
reportLifeCycle: () => {},
reportPluginLifeCycleLog: () => {},
reportPluginInteractionLog: () => {},
diff --git a/polygerrit-ui/app/services/gr-rest-api/gr-rest-api.ts b/polygerrit-ui/app/services/gr-rest-api/gr-rest-api.ts
index 947f518..b203715 100644
--- a/polygerrit-ui/app/services/gr-rest-api/gr-rest-api.ts
+++ b/polygerrit-ui/app/services/gr-rest-api/gr-rest-api.ts
@@ -16,6 +16,7 @@
*/
import {HttpMethod} from '../../constants/constants';
+import {Finalizable} from '../registry';
import {
AccountCapabilityInfo,
AccountDetailInfo,
@@ -129,7 +130,7 @@
comments: RobotCommentInfo[];
}
-export interface RestApiService {
+export interface RestApiService extends Finalizable {
getConfig(noCache?: boolean): Promise<ServerInfo | undefined>;
getLoggedIn(): Promise<boolean>;
getPreferences(): Promise<PreferencesInfo | undefined>;
diff --git a/polygerrit-ui/app/services/registry.ts b/polygerrit-ui/app/services/registry.ts
index 1e92c3c..ab204a2 100644
--- a/polygerrit-ui/app/services/registry.ts
+++ b/polygerrit-ui/app/services/registry.ts
@@ -15,30 +15,44 @@
* limitations under the License.
*/
+// A finalizable object has a single method `finalize` that is called when
+// the object is no longer needed and should clean itself up.
+export interface Finalizable {
+ finalize(): void;
+}
+
// A factory can take a partially created TContext and generate a property
// for a given key on that TContext.
export type Factory<TContext, K extends keyof TContext> = (
ctx: Partial<TContext>
-) => TContext[K];
+) => TContext[K] & Finalizable;
// A registry contains a factory for each key in TContext.
-export type Registry<TContext> = {[P in keyof TContext]: Factory<TContext, P>};
+export type Registry<TContext> = {[P in keyof TContext]: Factory<TContext, P>} &
+ Record<string, (_: TContext) => Finalizable>;
// Creates a context given a registry.
-// The cache parameter is a stop-gap solution because currently services do not
-// clean up after themselves. By passing in a cache, we can ensure that in
-// tests services are only created once.
export function create<TContext>(
- registry: Registry<TContext>,
- // eslint-disable-next-line @typescript-eslint/no-explicit-any
- cache?: Map<keyof TContext, any>
-): TContext {
- const context: Partial<TContext> = {} as Partial<TContext>;
- // eslint-disable-next-line @typescript-eslint/no-explicit-any
- const initialized: Map<keyof TContext, any> = cache
- ? cache
- : // eslint-disable-next-line @typescript-eslint/no-explicit-any
- new Map<keyof TContext, any>();
+ registry: Registry<TContext>
+): TContext & Finalizable {
+ const context: Partial<TContext> & Finalizable = {
+ finalize() {
+ for (const key of Object.getOwnPropertyNames(registry)) {
+ const name = key as keyof TContext;
+ try {
+ (this[name] as unknown as Finalizable).finalize();
+ } catch (e) {
+ console.info(`Failed to finalize ${name}`);
+ throw e;
+ }
+ }
+ },
+ } as Partial<TContext> & Finalizable;
+
+ const initialized: Map<keyof TContext, Finalizable> = new Map<
+ keyof TContext,
+ Finalizable
+ >();
for (const key of Object.keys(registry)) {
const name = key as keyof TContext;
const factory = registry[name];
@@ -55,7 +69,6 @@
if (initializing) throw new Error(`Circular dependency for ${key}`);
try {
initializing = true;
- console.info(`Initializing ${name}`);
initialized.set(name, factory(context));
} catch (e) {
console.error(`Failed to initialize ${name}`, e);
@@ -67,5 +80,5 @@
},
});
}
- return context as TContext;
+ return context as TContext & Finalizable;
}
diff --git a/polygerrit-ui/app/services/registry_test.ts b/polygerrit-ui/app/services/registry_test.ts
new file mode 100644
index 0000000..d677be0
--- /dev/null
+++ b/polygerrit-ui/app/services/registry_test.ts
@@ -0,0 +1,57 @@
+/**
+ * @license
+ * Copyright (C) 2021 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import {create, Finalizable, Registry} from './registry';
+import '../test/common-test-setup-karma.js';
+
+class Foo implements Finalizable {
+ constructor(private readonly final: string[]) {}
+
+ finalize() {
+ this.final.push('Foo');
+ }
+}
+
+class Bar implements Finalizable {
+ constructor(private readonly final: string[], _foo?: Foo) {}
+
+ finalize() {
+ this.final.push('Bar');
+ }
+}
+
+interface DemoContext {
+ foo: Foo;
+ bar: Bar;
+}
+
+suite('Registry', () => {
+ setup(() => {});
+
+ test('It finalizes correctly', () => {
+ const final: string[] = [];
+ const demoRegistry: Registry<DemoContext> = {
+ foo: (_ctx: Partial<DemoContext>) => new Foo(final),
+ bar: (ctx: Partial<DemoContext>) => new Bar(final, ctx.foo),
+ };
+ const demoContext: DemoContext & Finalizable = create<DemoContext>(
+ demoRegistry
+ ) as DemoContext & Finalizable;
+ demoContext.finalize();
+ assert.deepEqual(final, ['Foo', 'Bar']);
+ });
+});
diff --git a/polygerrit-ui/app/services/shortcuts/shortcuts-service.ts b/polygerrit-ui/app/services/shortcuts/shortcuts-service.ts
index f2e9e98..1f9e083 100644
--- a/polygerrit-ui/app/services/shortcuts/shortcuts-service.ts
+++ b/polygerrit-ui/app/services/shortcuts/shortcuts-service.ts
@@ -14,6 +14,7 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
+import {Subscription} from 'rxjs';
import {
config,
Shortcut,
@@ -31,6 +32,7 @@
shouldSuppress,
} from '../../utils/dom-util';
import {ReportingService} from '../gr-reporting/gr-reporting';
+import {Finalizable} from '../registry';
export type SectionView = Array<{binding: string[][]; text: string}>;
@@ -62,7 +64,7 @@
/**
* Shortcuts service, holds all hosts, bindings and listeners.
*/
-export class ShortcutsService {
+export class ShortcutsService implements Finalizable {
/**
* Keeps track of the components that are currently active such that we can
* show a shortcut help dialog that only shows the shortcuts that are
@@ -92,6 +94,10 @@
/** Keeps track of the corresponding user preference. */
private shortcutsDisabled = false;
+ private readonly keydownListener: (e: KeyboardEvent) => void;
+
+ private readonly subscriptions: Subscription[] = [];
+
constructor(readonly reporting?: ReportingService) {
for (const section of config.keys()) {
const items = config.get(section) ?? [];
@@ -99,12 +105,22 @@
this.bindings.set(item.shortcut, item.bindings);
}
}
- disableShortcuts$.subscribe(x => (this.shortcutsDisabled = x));
- document.addEventListener('keydown', (e: KeyboardEvent) => {
+ this.subscriptions.push(
+ disableShortcuts$.subscribe(x => (this.shortcutsDisabled = x))
+ );
+ this.keydownListener = (e: KeyboardEvent) => {
if (!isComboKey(e.key)) return;
if (this.shouldSuppress(e)) return;
this.comboKeyLastPressed = {key: e.key, timestampMs: Date.now()};
- });
+ };
+ document.addEventListener('keydown', this.keydownListener);
+ }
+
+ finalize() {
+ document.removeEventListener('keydown', this.keydownListener);
+ for (const s of this.subscriptions) {
+ s.unsubscribe();
+ }
}
public _testOnly_isEmpty() {
diff --git a/polygerrit-ui/app/services/storage/gr-storage.ts b/polygerrit-ui/app/services/storage/gr-storage.ts
index 08a3387..0b995d8 100644
--- a/polygerrit-ui/app/services/storage/gr-storage.ts
+++ b/polygerrit-ui/app/services/storage/gr-storage.ts
@@ -16,6 +16,7 @@
*/
import {CommentRange, PatchSetNum} from '../../types/common';
+import {Finalizable} from '../registry';
export interface StorageLocation {
changeNum: number;
@@ -30,7 +31,7 @@
updated: number;
}
-export interface StorageService {
+export interface StorageService extends Finalizable {
getDraftComment(location: StorageLocation): StorageObject | null;
setDraftComment(location: StorageLocation, message: string): void;
diff --git a/polygerrit-ui/app/services/storage/gr-storage_impl.ts b/polygerrit-ui/app/services/storage/gr-storage_impl.ts
index 0c0d151..0824fb4 100644
--- a/polygerrit-ui/app/services/storage/gr-storage_impl.ts
+++ b/polygerrit-ui/app/services/storage/gr-storage_impl.ts
@@ -16,6 +16,7 @@
*/
import {StorageLocation, StorageObject, StorageService} from './gr-storage';
+import {Finalizable} from '../registry';
export const DURATION_DAY = 24 * 60 * 60 * 1000;
@@ -27,13 +28,15 @@
CLEANUP_PREFIXES_MAX_AGE_MAP.set('draft', DURATION_DAY);
CLEANUP_PREFIXES_MAX_AGE_MAP.set('editablecontent', DURATION_DAY);
-export class GrStorageService implements StorageService {
+export class GrStorageService implements StorageService, Finalizable {
private lastCleanup = 0;
private readonly storage = window.localStorage;
private exceededQuota = false;
+ finalize() {}
+
getDraftComment(location: StorageLocation): StorageObject | null {
this.cleanupItems();
return this.getObject(this.getDraftKey(location));
diff --git a/polygerrit-ui/app/services/storage/gr-storage_mock.ts b/polygerrit-ui/app/services/storage/gr-storage_mock.ts
index 399ffe4..79f0fbc 100644
--- a/polygerrit-ui/app/services/storage/gr-storage_mock.ts
+++ b/polygerrit-ui/app/services/storage/gr-storage_mock.ts
@@ -45,6 +45,7 @@
}
export const grStorageMock: StorageService = {
+ finalize(): void {},
getDraftComment(location: StorageLocation): StorageObject | null {
return storage.get(getDraftKey(location)) ?? null;
},
diff --git a/polygerrit-ui/app/services/user/user-service.ts b/polygerrit-ui/app/services/user/user-service.ts
index d08da8b..d2bca85 100644
--- a/polygerrit-ui/app/services/user/user-service.ts
+++ b/polygerrit-ui/app/services/user/user-service.ts
@@ -14,49 +14,63 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
+import {from, of, Subscription} from 'rxjs';
+import {switchMap} from 'rxjs/operators';
import {AccountDetailInfo, PreferencesInfo} from '../../types/common';
-import {from, of} from 'rxjs';
import {
account$,
updateAccount,
updatePreferences,
updateDiffPreferences,
} from './user-model';
-import {switchMap} from 'rxjs/operators';
import {
createDefaultPreferences,
createDefaultDiffPrefs,
} from '../../constants/constants';
import {RestApiService} from '../gr-rest-api/gr-rest-api';
import {DiffPreferencesInfo} from '../../types/diff';
+import {Finalizable} from '../registry';
-export class UserService {
+export class UserService implements Finalizable {
+ private readonly subscriptions: Subscription[] = [];
+
constructor(readonly restApiService: RestApiService) {
from(this.restApiService.getAccount()).subscribe(
(account?: AccountDetailInfo) => {
updateAccount(account);
}
);
- account$
- .pipe(
- switchMap(account => {
- if (!account) return of(createDefaultPreferences());
- return from(this.restApiService.getPreferences());
+ this.subscriptions.push(
+ account$
+ .pipe(
+ switchMap(account => {
+ if (!account) return of(createDefaultPreferences());
+ return from(this.restApiService.getPreferences());
+ })
+ )
+ .subscribe((preferences?: PreferencesInfo) => {
+ updatePreferences(preferences ?? createDefaultPreferences());
})
- )
- .subscribe((preferences?: PreferencesInfo) => {
- updatePreferences(preferences ?? createDefaultPreferences());
- });
- account$
- .pipe(
- switchMap(account => {
- if (!account) return of(createDefaultDiffPrefs());
- return from(this.restApiService.getDiffPreferences());
+ );
+ this.subscriptions.push(
+ account$
+ .pipe(
+ switchMap(account => {
+ if (!account) return of(createDefaultDiffPrefs());
+ return from(this.restApiService.getDiffPreferences());
+ })
+ )
+ .subscribe((diffPrefs?: DiffPreferencesInfo) => {
+ updateDiffPreferences(diffPrefs ?? createDefaultDiffPrefs());
})
- )
- .subscribe((diffPrefs?: DiffPreferencesInfo) => {
- updateDiffPreferences(diffPrefs ?? createDefaultDiffPrefs());
- });
+ );
+ }
+
+ finalize() {
+ for (const s of this.subscriptions) {
+ s.unsubscribe();
+ }
+ this.subscriptions.splice(0, this.subscriptions.length);
}
updatePreferences(prefs: Partial<PreferencesInfo>) {
diff --git a/polygerrit-ui/app/test/common-test-setup-karma.ts b/polygerrit-ui/app/test/common-test-setup-karma.ts
index 39c79d1..888ace0 100644
--- a/polygerrit-ui/app/test/common-test-setup-karma.ts
+++ b/polygerrit-ui/app/test/common-test-setup-karma.ts
@@ -36,6 +36,7 @@
// For uncaught error mochajs doesn't print the full stack trace.
// We should print it ourselves.
console.error('Uncaught error:');
+ console.error(e);
console.error(e.error.stack.toString());
unhandledError = e;
});
diff --git a/polygerrit-ui/app/test/common-test-setup.ts b/polygerrit-ui/app/test/common-test-setup.ts
index 060bb8b..f555e8e 100644
--- a/polygerrit-ui/app/test/common-test-setup.ts
+++ b/polygerrit-ui/app/test/common-test-setup.ts
@@ -21,7 +21,10 @@
import '../scripts/bundled-polymer';
import '@polymer/iron-test-helpers/iron-test-helpers';
import './test-router';
-import {_testOnlyInitAppContext} from './test-app-context-init';
+import {
+ _testOnlyInitAppContext,
+ _testOnlyFinalizeAppContext,
+} from './test-app-context-init';
import {_testOnly_resetPluginLoader} from '../elements/shared/gr-js-api-interface/gr-plugin-loader';
import {_testOnlyResetGrRestApiSharedObjects} from '../elements/shared/gr-rest-api-interface/gr-rest-api-interface';
import {
@@ -220,6 +223,7 @@
cleanUpStorage();
// Reset state
updatePreferences(createDefaultPreferences());
+ _testOnlyFinalizeAppContext();
const testTeardownTimestampMs = new Date().getTime();
const elapsedMs = testTeardownTimestampMs - testSetupTimestampMs;
if (elapsedMs > 1000) {
diff --git a/polygerrit-ui/app/test/mocks/gr-rest-api_mock.ts b/polygerrit-ui/app/test/mocks/gr-rest-api_mock.ts
index 63ffcbd..dfc0a0b 100644
--- a/polygerrit-ui/app/test/mocks/gr-rest-api_mock.ts
+++ b/polygerrit-ui/app/test/mocks/gr-rest-api_mock.ts
@@ -170,6 +170,7 @@
executeChangeAction(): Promise<Response | undefined> {
return Promise.resolve(new Response());
},
+ finalize(): void {},
generateAccountHttpPassword(): Promise<Password> {
return Promise.resolve('asdf');
},
@@ -276,16 +277,22 @@
throw new Error('getDiffChangeDetail() not implemented by RestApiMock.');
},
getDiffComments() {
- throw new Error('getDiffComments() not implemented by RestApiMock.');
+ // NOTE: This method can not be typed properly due to overloads.
+ // eslint-disable-next-line @typescript-eslint/no-explicit-any
+ return Promise.resolve({}) as any;
},
getDiffDrafts() {
- throw new Error('getDiffDrafts() not implemented by RestApiMock.');
+ // NOTE: This method can not be typed properly due to overloads.
+ // eslint-disable-next-line @typescript-eslint/no-explicit-any
+ return Promise.resolve({}) as any;
},
getDiffPreferences(): Promise<DiffPreferencesInfo | undefined> {
return Promise.resolve(createDefaultDiffPrefs());
},
getDiffRobotComments() {
- throw new Error('getDiffRobotComments() not implemented by RestApiMock.');
+ // NOTE: This method can not be typed properly due to overloads.
+ // eslint-disable-next-line @typescript-eslint/no-explicit-any
+ return Promise.resolve({}) as any;
},
getDocumentationSearches(): Promise<DocResult[] | undefined> {
return Promise.resolve([]);
diff --git a/polygerrit-ui/app/test/test-app-context-init.ts b/polygerrit-ui/app/test/test-app-context-init.ts
index 04736ec..2817d35 100644
--- a/polygerrit-ui/app/test/test-app-context-init.ts
+++ b/polygerrit-ui/app/test/test-app-context-init.ts
@@ -16,28 +16,73 @@
*/
// Init app context before any other imports
-import {createAppContext} from '../services/app-context-init';
-import {grReportingMock} from '../services/gr-reporting/gr-reporting_mock';
+import {create, Registry, Finalizable} from '../services/registry';
+import {assertIsDefined} from '../utils/common-util';
import {AppContext, injectAppContext} from '../services/app-context';
+import {grReportingMock} from '../services/gr-reporting/gr-reporting_mock';
import {grRestApiMock} from './mocks/gr-rest-api_mock';
import {grStorageMock} from '../services/storage/gr-storage_mock';
import {GrAuthMock} from '../services/gr-auth/gr-auth_mock';
-export function _testOnlyInitAppContext() {
- const appContext = createAppContext();
- injectAppContext(appContext);
+import {FlagsServiceImplementation} from '../services/flags/flags_impl';
+import {EventEmitter} from '../services/gr-event-interface/gr-event-interface_impl';
+import {ChangeService} from '../services/change/change-service';
+import {ChecksService} from '../services/checks/checks-service';
+import {GrJsApiInterface} from '../elements/shared/gr-js-api-interface/gr-js-api-interface-element';
+import {ConfigService} from '../services/config/config-service';
+import {UserService} from '../services/user/user-service';
+import {CommentsService} from '../services/comments/comments-service';
+import {ShortcutsService} from '../services/shortcuts/shortcuts-service';
+import {BrowserService} from '../services/browser/browser-service';
- function setMock<T extends keyof AppContext>(
- serviceName: T,
- setupMock: AppContext[T]
- ) {
- Object.defineProperty(appContext, serviceName, {
- get() {
- return setupMock;
- },
- });
- }
- setMock('reportingService', grReportingMock);
- setMock('restApiService', grRestApiMock);
- setMock('storageService', grStorageMock);
- setMock('authService', new GrAuthMock(appContext.eventEmitter));
+let appContext: (AppContext & Finalizable) | undefined;
+
+export function _testOnlyInitAppContext() {
+ const appRegistry: Registry<AppContext> = {
+ flagsService: (_ctx: Partial<AppContext>) =>
+ new FlagsServiceImplementation(),
+ reportingService: (_ctx: Partial<AppContext>) => grReportingMock,
+ eventEmitter: (_ctx: Partial<AppContext>) => new EventEmitter(),
+ authService: (ctx: Partial<AppContext>) => {
+ assertIsDefined(ctx.eventEmitter, 'eventEmitter');
+ return new GrAuthMock(ctx.eventEmitter);
+ },
+ restApiService: (_ctx: Partial<AppContext>) => grRestApiMock,
+ changeService: (ctx: Partial<AppContext>) => {
+ assertIsDefined(ctx.restApiService, 'restApiService');
+ return new ChangeService(ctx.restApiService!);
+ },
+ commentsService: (ctx: Partial<AppContext>) => {
+ assertIsDefined(ctx.restApiService, 'restApiService');
+ return new CommentsService(ctx.restApiService!);
+ },
+ checksService: (ctx: Partial<AppContext>) => {
+ assertIsDefined(ctx.reportingService, 'reportingService');
+ return new ChecksService(ctx.reportingService!);
+ },
+ jsApiService: (ctx: Partial<AppContext>) => {
+ assertIsDefined(ctx.reportingService, 'reportingService');
+ return new GrJsApiInterface(ctx.reportingService!);
+ },
+ storageService: (_ctx: Partial<AppContext>) => grStorageMock,
+ configService: (ctx: Partial<AppContext>) => {
+ assertIsDefined(ctx.restApiService, 'restApiService');
+ return new ConfigService(ctx.restApiService!);
+ },
+ userService: (ctx: Partial<AppContext>) => {
+ assertIsDefined(ctx.restApiService, 'restApiService');
+ return new UserService(ctx.restApiService!);
+ },
+ shortcutsService: (ctx: Partial<AppContext>) => {
+ assertIsDefined(ctx.reportingService, 'reportingService');
+ return new ShortcutsService(ctx.reportingService!);
+ },
+ browserService: (_ctx: Partial<AppContext>) => new BrowserService(),
+ };
+ appContext = create<AppContext>(appRegistry);
+ injectAppContext(appContext);
+}
+
+export function _testOnlyFinalizeAppContext() {
+ appContext?.finalize();
+ appContext = undefined;
}
diff --git a/polygerrit-ui/app/utils/async-util.ts b/polygerrit-ui/app/utils/async-util.ts
index 93e3362..3f51532 100644
--- a/polygerrit-ui/app/utils/async-util.ts
+++ b/polygerrit-ui/app/utils/async-util.ts
@@ -139,7 +139,9 @@
*/
export function until<T>(obs$: Observable<T>, predicate: (t: T) => boolean) {
return new Promise<void>(resolve => {
- obs$.pipe(filter(predicate), take(1)).subscribe(() => resolve());
+ obs$.pipe(filter(predicate), take(1)).subscribe(() => {
+ resolve();
+ });
});
}
