Get rid of logging for autoclose bug. Release-Notes: skip Change-Id: I99bacf02c4d01699e13c7ec10949dde6a1ca5b4d
diff --git a/polygerrit-ui/app/constants/reporting.ts b/polygerrit-ui/app/constants/reporting.ts index be86863..92d6bdf 100644 --- a/polygerrit-ui/app/constants/reporting.ts +++ b/polygerrit-ui/app/constants/reporting.ts
@@ -124,32 +124,5 @@ CHECKS_RUNS_PANEL_TOGGLE = 'checks-runs-panel-toggle', CHECKS_RUNS_SELECTED_TRIGGERED = 'checks-runs-selected-triggered', CHECKS_STATS = 'checks-stats', - // The following interactions are logged for investigating a spurious bug of - // auto-closing draft comments. - COMMENTS_AUTOCLOSE_FIRST_UPDATE = 'comments-autoclose-first-update', - COMMENTS_AUTOCLOSE_EDITING_FALSE_SAVE = 'comments-autoclose-editing-false-save', - COMMENTS_AUTOCLOSE_EDITING_DISCONNECTED = 'comments-autoclose-editing-disconnected', - COMMENTS_AUTOCLOSE_EDITING_THREAD_DISCONNECTED = 'comments-autoclose-editing-thread-disconnected', - COMMENTS_AUTOCLOSE_CHECKS_UPDATED = 'comments-autoclose-checks-updated', - COMMENTS_AUTOCLOSE_THREADS_UPDATED = 'comments-autoclose-threads-updated', - COMMENTS_AUTOCLOSE_COMMENT_REMOVED = 'comments-autoclose-comment-removed', - COMMENTS_AUTOCLOSE_MESSAGES_LIST_CREATED = 'comments-autoclose-messages-list-created', - COMMENTS_AUTOCLOSE_MESSAGES_LIST_UPDATED = 'comments-autoclose-messages-list-updated', - COMMENTS_AUTOCLOSE_THREAD_LIST_CREATED = 'comments-autoclose-thread-list-created', - COMMENTS_AUTOCLOSE_THREAD_LIST_UPDATED = 'comments-autoclose-thread-list-updated', - // The following interactions are logged for investigating a spurious bug of - // auto-closing diffs. - DIFF_AUTOCLOSE_DIFF_UNDEFINED = 'diff-autoclose-diff-undefined', - DIFF_AUTOCLOSE_DIFF_ONGOING = 'diff-autoclose-diff-ongoing', - DIFF_AUTOCLOSE_RELOAD_ON_WHITESPACE = 'diff-autoclose-reload-on-whitespace', - DIFF_AUTOCLOSE_RELOAD_ON_SYNTAX = 'diff-autoclose-reload-on-syntax', - DIFF_AUTOCLOSE_RELOAD_FILELIST_PREFS = 'diff-autoclose-reload-filelist-prefs', - DIFF_AUTOCLOSE_DIFF_HOST_CREATED = 'diff-autoclose-diff-host-created', - DIFF_AUTOCLOSE_DIFF_HOST_NOT_RENDERING = 'diff-autoclose-diff-not-rendering', - DIFF_AUTOCLOSE_FILE_LIST_UPDATED = 'diff-autoclose-file-list-updated', - DIFF_AUTOCLOSE_SHOWN_FILES_CHANGED = 'diff-autoclose-shown-files-changed', - // The following interaction is logged for reporting and counting a suspected - // Chrome bug that leads to html`` misbehavior. - AUTOCLOSE_HTML_PATCHED = 'autoclose-html-patched', CHANGE_ACTION_FIRED = 'change-action-fired', }
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 ad75253..999190e 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
@@ -51,7 +51,7 @@ import {GrCursorManager} from '../../shared/gr-cursor-manager/gr-cursor-manager'; import {ChangeComments} from '../../diff/gr-comment-api/gr-comment-api'; import {ParsedChangeInfo, PatchSetFile} from '../../../types/types'; -import {Interaction, Timing} from '../../../constants/reporting'; +import {Timing} from '../../../constants/reporting'; import {RevisionInfo} from '../../shared/revision-info/revision-info'; import {select} from '../../../utils/observable-util'; import {resolve} from '../../../models/dependency'; @@ -78,7 +78,6 @@ import {classMap} from 'lit/directives/class-map.js'; import {incrementalRepeat} from '../../lit/incremental-repeat'; import {ifDefined} from 'lit/directives/if-defined.js'; -import {HtmlPatched} from '../../../utils/lit-util'; import { createDiffUrl, createEditUrl, @@ -309,13 +308,6 @@ private readonly getBrowserModel = resolve(this, browserModelToken); - private readonly patched = new HtmlPatched(key => { - this.reporting.reportInteraction(Interaction.AUTOCLOSE_HTML_PATCHED, { - component: this.tagName, - key: key.substring(0, 300), - }); - }); - shortcutsController = new ShortcutController(this); private readonly getNavigation = resolve(this, navigationToken); @@ -1015,25 +1007,12 @@ ); } - // for DIFF_AUTOCLOSE logging purposes only - private shownFilesOld: NormalizedFileInfo[] = this.shownFiles; - private renderShownFiles() { const showDynamicColumns = this.computeShowDynamicColumns(); const showPrependedDynamicColumns = this.computeShowPrependedDynamicColumns(); const sizeBarLayout = this.computeSizeBarLayout(); - // for DIFF_AUTOCLOSE logging purposes only - if ( - this.shownFilesOld.length > 0 && - this.shownFiles !== this.shownFilesOld - ) { - this.reporting.reportInteraction( - Interaction.DIFF_AUTOCLOSE_SHOWN_FILES_CHANGED - ); - } - this.shownFilesOld = this.shownFiles; return incrementalRepeat({ values: this.shownFiles, mapFn: (f, i) => @@ -1084,7 +1063,7 @@ </div> ${when( this.isFileExpanded(file.__path), - () => this.patched.html` + () => html` <gr-diff-host ?noAutoRender=${true} ?showLoadFailure=${true} @@ -1658,17 +1637,6 @@ this.reporting.fileListDisplayed(); } - protected override updated(): void { - // for DIFF_AUTOCLOSE logging purposes only - const ids = this.diffs.map(d => d.uid); - if (ids.length > 0) { - this.reporting.reportInteraction( - Interaction.DIFF_AUTOCLOSE_FILE_LIST_UPDATED, - {l: ids.length, ids: ids.slice(0, 10)} - ); - } - } - // TODO: Move into files-model. // visible for testing async updateCleanlyMergedPaths() { @@ -1787,10 +1755,6 @@ if (!this.diffs.length) { return; } - this.reporting.reportInteraction( - Interaction.DIFF_AUTOCLOSE_RELOAD_FILELIST_PREFS - ); - // Re-render all expanded diffs sequentially. this.renderInOrder(this.expandedFiles, this.diffs); }
diff --git a/polygerrit-ui/app/elements/change/gr-message/gr-message.ts b/polygerrit-ui/app/elements/change/gr-message/gr-message.ts index 72969a7..45f24d7 100644 --- a/polygerrit-ui/app/elements/change/gr-message/gr-message.ts +++ b/polygerrit-ui/app/elements/change/gr-message/gr-message.ts
@@ -128,9 +128,6 @@ private readonly getNavigation = resolve(this, navigationToken); - // for COMMENTS_AUTOCLOSE logging purposes only - readonly uid = performance.now().toString(36) + Math.random().toString(36); - constructor() { super(); this.addEventListener('click', e => this.handleClick(e));
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 c46f4fc..972b871 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
@@ -43,7 +43,6 @@ shortcutsServiceToken, } from '../../../services/shortcuts/shortcuts-service'; import {GrFormattedText} from '../../shared/gr-formatted-text/gr-formatted-text'; -import {Interaction} from '../../../constants/reporting'; /** * The content of the enum is also used in the UI for the button text. @@ -353,21 +352,6 @@ this.changeNum = x; } ); - // for COMMENTS_AUTOCLOSE logging purposes only - this.reporting.reportInteraction( - Interaction.COMMENTS_AUTOCLOSE_MESSAGES_LIST_CREATED - ); - } - - override updated(): void { - // for COMMENTS_AUTOCLOSE logging purposes only - const messages = this.shadowRoot!.querySelectorAll('gr-message'); - if (messages.length > 0) { - this.reporting.reportInteraction( - Interaction.COMMENTS_AUTOCLOSE_MESSAGES_LIST_UPDATED, - {uid: messages[0].uid} - ); - } } override willUpdate(changedProperties: PropertyValues): void {
diff --git a/polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list.ts b/polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list.ts index 599e38b..7015900 100644 --- a/polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list.ts +++ b/polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list.ts
@@ -42,11 +42,8 @@ import {ParsedChangeInfo} from '../../../types/types'; import {repeat} from 'lit/directives/repeat.js'; import {GrCommentThread} from '../../shared/gr-comment-thread/gr-comment-thread'; -import {getAppContext} from '../../../services/app-context'; import {resolve} from '../../../models/dependency'; import {changeModelToken} from '../../../models/change/change-model'; -import {Interaction} from '../../../constants/reporting'; -import {HtmlPatched} from '../../../utils/lit-util'; import {userModelToken} from '../../../models/user/user-model'; import {specialFilePathCompare} from '../../../utils/path-list-util'; @@ -206,17 +203,8 @@ private readonly getChangeModel = resolve(this, changeModelToken); - private readonly reporting = getAppContext().reportingService; - private readonly getUserModel = resolve(this, userModelToken); - private readonly patched = new HtmlPatched(key => { - this.reporting.reportInteraction(Interaction.AUTOCLOSE_HTML_PATCHED, { - component: this.tagName, - key: key.substring(0, 300), - }); - }); - constructor() { super(); subscribe( @@ -234,10 +222,6 @@ () => this.getUserModel().account$, x => (this.account = x) ); - // for COMMENTS_AUTOCLOSE logging purposes only - this.reporting.reportInteraction( - Interaction.COMMENTS_AUTOCLOSE_THREAD_LIST_CREATED - ); } override willUpdate(changed: PropertyValues) { @@ -341,17 +325,6 @@ ]; } - override updated(): void { - // for COMMENTS_AUTOCLOSE logging purposes only - const threads = this.shadowRoot!.querySelectorAll('gr-comment-thread'); - if (threads.length > 0) { - this.reporting.reportInteraction( - Interaction.COMMENTS_AUTOCLOSE_THREAD_LIST_UPDATED, - {uid: threads[0].uid} - ); - } - } - override render() { return html` ${this.renderDropdown()} @@ -425,16 +398,16 @@ index === 0 || threads[index - 1].path !== threads[index].path; const separator = index !== 0 && isFirst - ? this.patched.html`<div class="thread-separator"></div>` + ? html`<div class="thread-separator"></div>` : undefined; const commentThread = this.renderCommentThread(thread, isFirst); - return this.patched.html`${separator}${commentThread}`; + return html`${separator}${commentThread}`; } ); } private renderCommentThread(thread: CommentThread, isFirst: boolean) { - return this.patched.html` + return html` <gr-comment-thread .thread=${thread} show-file-path
diff --git a/polygerrit-ui/app/elements/checks/gr-checks-results.ts b/polygerrit-ui/app/elements/checks/gr-checks-results.ts index f3be5c4..5071e4f 100644 --- a/polygerrit-ui/app/elements/checks/gr-checks-results.ts +++ b/polygerrit-ui/app/elements/checks/gr-checks-results.ts
@@ -72,7 +72,6 @@ import {changeModelToken} from '../../models/change/change-model'; import {getAppContext} from '../../services/app-context'; import {when} from 'lit/directives/when.js'; -import {HtmlPatched} from '../../utils/lit-util'; import {DropdownItem} from '../shared/gr-dropdown-list/gr-dropdown-list'; import './gr-checks-attempt'; import {createDiffUrl, changeViewModelToken} from '../../models/views/change'; @@ -818,13 +817,6 @@ private readonly reporting = getAppContext().reportingService; - private readonly patched = new HtmlPatched(key => { - this.reporting.reportInteraction(Interaction.AUTOCLOSE_HTML_PATCHED, { - component: this.tagName, - key: key.substring(0, 300), - }); - }); - constructor() { super(); subscribe( @@ -1502,7 +1494,7 @@ ${repeat( filtered, result => result.internalResultId, - (result?: RunResult) => this.patched.html` + (result?: RunResult) => html` <gr-result-row class=${charsOnly(result!.checkName)} .result=${result}
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 43a6819..0fa5503 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
@@ -64,7 +64,7 @@ import {assertIsDefined} from '../../../utils/common-util'; import {DiffContextExpandedEventDetail} from '../../../embed/diff/gr-diff-builder/gr-diff-builder'; import {TokenHighlightLayer} from '../../../embed/diff/gr-diff-builder/token-highlight-layer'; -import {Timing, Interaction} from '../../../constants/reporting'; +import {Timing} from '../../../constants/reporting'; import {ChangeComments} from '../gr-comment-api/gr-comment-api'; import {Subscription} from 'rxjs'; import { @@ -336,9 +336,6 @@ private checksSubscription?: Subscription; - // for DIFF_AUTOCLOSE logging purposes only - readonly uid = performance.now().toString(36) + Math.random().toString(36); - constructor() { super(); this.syntaxLayer = new GrSyntaxLayerWorker( @@ -386,23 +383,6 @@ this.prefs = diffPreferences; } ); - this.logForDiffAutoClose(); - } - - // for DIFF_AUTOCLOSE logging purposes only - private logForDiffAutoClose() { - this.reporting.reportInteraction( - Interaction.DIFF_AUTOCLOSE_DIFF_HOST_CREATED, - {uid: this.uid} - ); - setTimeout(() => { - if (!this.hasReloadBeenCalledOnce) { - this.reporting.reportInteraction( - Interaction.DIFF_AUTOCLOSE_DIFF_HOST_NOT_RENDERING, - {uid: this.uid} - ); - } - }, /* 10 seconds */ 10000); } override connectedCallback() { @@ -586,14 +566,7 @@ return this.reloadPromise; } - // for DIFF_AUTOCLOSE logging purposes only - private reloadOngoing = false; - - // for DIFF_AUTOCLOSE logging purposes only - private hasReloadBeenCalledOnce = false; - async reloadInternal(shouldReportMetric?: boolean) { - this.hasReloadBeenCalledOnce = true; this.reporting.time(Timing.DIFF_TOTAL); this.reporting.time(Timing.DIFF_LOAD); // TODO: Find better names for these 3 clear/cancel methods. Ideally the @@ -606,10 +579,6 @@ this.diff = undefined; this.errorMessage = null; const whitespaceLevel = this.getIgnoreWhitespace(); - if (this.reloadOngoing) { - this.reporting.reportInteraction(Interaction.DIFF_AUTOCLOSE_DIFF_ONGOING); - } - this.reloadOngoing = true; try { // We are carefully orchestrating operations that have to wait for another @@ -619,11 +588,6 @@ // assets in parallel. const layerPromise = this.initLayers(); const diff = await this.getDiff(); - if (diff === undefined) { - this.reporting.reportInteraction( - Interaction.DIFF_AUTOCLOSE_DIFF_UNDEFINED - ); - } this.loadedWhitespaceLevel = whitespaceLevel; this.reportDiff(diff); @@ -670,7 +634,6 @@ } } finally { this.reporting.timeEnd(Timing.DIFF_TOTAL, this.timingDetails()); - this.reloadOngoing = false; } } @@ -758,9 +721,6 @@ const idToEl = new Map<string, GrDiffCheckResult>(); const checkEls = this.getCheckEls(); const dontRemove = new Set<GrDiffCheckResult>(); - let createdCount = 0; - let updatedCount = 0; - let removedCount = 0; const checksCount = checks.length; const checkElsCount = checkEls.length; if (checksCount === 0 && checkElsCount === 0) return; @@ -775,23 +735,16 @@ if (existingEl) { existingEl.result = check; dontRemove.add(existingEl); - updatedCount++; } else { const newEl = this.createCheckEl(check); dontRemove.add(newEl); - createdCount++; } } // Remove all check els that don't have a matching check anymore. for (const el of checkEls) { if (dontRemove.has(el)) continue; el.remove(); - removedCount++; } - this.reporting.reportInteraction( - Interaction.COMMENTS_AUTOCLOSE_CHECKS_UPDATED, - {createdCount, updatedCount, removedCount, checksCount, checkElsCount} - ); } /** @@ -1106,9 +1059,6 @@ } } const dontRemove = new Set<GrCommentThread>(); - let createdCount = 0; - let updatedCount = 0; - let removedCount = 0; const threadCount = threads.length; const threadElCount = threadEls.length; if (threadCount === 0 && threadElCount === 0) return; @@ -1150,12 +1100,10 @@ ) { existingThreadEl.thread = thread; dontRemove.add(existingThreadEl); - updatedCount++; } else { const threadEl = this.createThreadElement(thread); this.attachThreadElement(threadEl); dontRemove.add(threadEl); - createdCount++; } } // Remove all threads that are no longer existing. @@ -1165,13 +1113,8 @@ // might be unsaved and thus not be reflected in `threads` yet, so let's // keep them open. if (threadEl.editing && threadEl.thread?.comments.length === 0) continue; - removedCount++; threadEl.remove(); } - this.reporting.reportInteraction( - Interaction.COMMENTS_AUTOCLOSE_THREADS_UPDATED, - {createdCount, updatedCount, removedCount, threadCount, threadElCount} - ); const portedThreadsCount = threads.filter(thread => thread.ported).length; const portedThreadsWithoutRange = threads.filter( thread => thread.ported && thread.rangeInfoLost @@ -1371,9 +1314,6 @@ preferredWhitespaceLevel !== loadedWhitespaceLevel && !noRenderOnPrefsChange ) { - this.reporting.reportInteraction( - Interaction.DIFF_AUTOCLOSE_RELOAD_ON_WHITESPACE - ); return this.reload(); } } @@ -1392,9 +1332,6 @@ if (oldPrefs?.syntax_highlighting === prefs.syntax_highlighting) return; if (!noRenderOnPrefsChange) { - this.reporting.reportInteraction( - Interaction.DIFF_AUTOCLOSE_RELOAD_ON_SYNTAX - ); return this.reload(); } }
diff --git a/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread.ts b/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread.ts index 062aa54..aba0aea 100644 --- a/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread.ts +++ b/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread.ts
@@ -70,8 +70,6 @@ import {commentsModelToken} from '../../../models/comments/comments-model'; import {changeModelToken} from '../../../models/change/change-model'; import {whenRendered} from '../../../utils/dom-util'; -import {Interaction} from '../../../constants/reporting'; -import {HtmlPatched} from '../../../utils/lit-util'; import {createChangeUrl, createDiffUrl} from '../../../models/views/change'; import {userModelToken} from '../../../models/user/user-model'; import {highlightServiceToken} from '../../../services/highlight/highlight-service'; @@ -254,8 +252,6 @@ private readonly getUserModel = resolve(this, userModelToken); - private readonly reporting = getAppContext().reportingService; - private readonly shortcuts = new ShortcutController(this); private readonly syntaxLayer = new GrSyntaxLayerWorker( @@ -263,16 +259,6 @@ () => getAppContext().reportingService ); - // for COMMENTS_AUTOCLOSE logging purposes only - readonly uid = performance.now().toString(36) + Math.random().toString(36); - - private readonly patched = new HtmlPatched(key => { - this.reporting.reportInteraction(Interaction.AUTOCLOSE_HTML_PATCHED, { - component: this.tagName, - key: key.substring(0, 300), - }); - }); - constructor() { super(); this.shortcuts.addGlobal({key: 'e'}, () => this.handleExpandShortcut()); @@ -322,15 +308,6 @@ ); } - override disconnectedCallback() { - if (this.editing) { - this.reporting.reportInteraction( - Interaction.COMMENTS_AUTOCLOSE_EDITING_THREAD_DISCONNECTED - ); - } - super.disconnectedCallback(); - } - static override get styles() { return [ a11yStyles, @@ -519,16 +496,15 @@ (this.messageId ? comment.change_message_id !== this.messageId : !this.unresolved); - return this.patched.html` + return html` <gr-comment .comment=${comment} .comments=${this.thread!.comments} ?initially-collapsed=${initiallyCollapsed} ?robot-button-disabled=${robotButtonDisabled} ?show-patchset=${this.showPatchset} - ?show-ported-comment=${ - this.showPortedComment && comment.id === this.rootId - } + ?show-ported-comment=${this.showPortedComment && + comment.id === this.rootId} @reply-to-comment=${this.handleReplyToComment} @copy-comment-link=${this.handleCopyLink} @comment-editing-changed=${(
diff --git a/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts b/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts index 0edfe03..f7e8edd 100644 --- a/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts +++ b/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts
@@ -57,7 +57,6 @@ import {Subject} from 'rxjs'; import {debounceTime} from 'rxjs/operators'; import {changeModelToken} from '../../../models/change/change-model'; -import {Interaction} from '../../../constants/reporting'; import {KnownExperimentId} from '../../../services/flags/flags'; import {isBase64FileContent} from '../../../api/rest-api'; import {createDiffUrl} from '../../../models/views/change'; @@ -318,11 +317,6 @@ override disconnectedCallback() { // Clean up emoji dropdown. if (this.textarea) this.textarea.closeDropdown(); - if (this.editing) { - this.reporting.reportInteraction( - Interaction.COMMENTS_AUTOCLOSE_EDITING_DISCONNECTED - ); - } super.disconnectedCallback(); } @@ -938,10 +932,6 @@ this.unresolved = this.comment.unresolved ?? true; if (isUnsaved(this.comment)) this.editing = true; if (isDraftOrUnsaved(this.comment)) { - this.reporting.reportInteraction( - Interaction.COMMENTS_AUTOCLOSE_FIRST_UPDATE, - {editing: this.editing, unsaved: isUnsaved(this.comment)} - ); this.collapsed = false; } else { this.collapsed = !!this.initiallyCollapsed; @@ -1210,9 +1200,6 @@ await this.rawSave(messageToSave, {showToast: true}); } } - this.reporting.reportInteraction( - Interaction.COMMENTS_AUTOCLOSE_EDITING_FALSE_SAVE - ); if (!this.permanentEditingMode) { this.editing = false; }
diff --git a/polygerrit-ui/app/utils/lit-util.ts b/polygerrit-ui/app/utils/lit-util.ts deleted file mode 100644 index 7ffab89..0000000 --- a/polygerrit-ui/app/utils/lit-util.ts +++ /dev/null
@@ -1,69 +0,0 @@ -/** - * @license - * Copyright 2022 Google LLC - * SPDX-License-Identifier: Apache-2.0 - */ -import {html} from 'lit'; - -/** - * This is a patched version of html`` to work around this Chrome bug: - * https://bugs.chromium.org/p/v8/issues/detail?id=13190. - * - * The problem is that Chrome should guarantee that the TemplateStringsArray - * is always the same instance, if the strings themselves are equal, but that - * guarantee seems to be broken. So we are maintaining a map from - * "concatenated strings" to TemplateStringsArray. If "concatenated strings" - * are equal, then return the already known instance of TemplateStringsArray, - * so html`` can use its strict equality check on it. - */ -export class HtmlPatched { - constructor(private readonly reporter?: (key: string) => void) {} - - /** - * If `strings` are in this set, then we are sure that they are also in the - * map, and that we will not run into the issue of "same key, but different - * strings array". So this set allows us to optimize performance a bit, and - * call the native html`` function early. - */ - private readonly lookupSet = new Set<TemplateStringsArray>(); - - private readonly lookupMap = new Map<string, TemplateStringsArray>(); - - /** - * Proxies lit's html`` tagges template literal. See - * https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Template_literals - * https://lit.dev/docs/libraries/standalone-templates/ - * - * Example: If you call html`a${1}b${2}c`, then - * ['a', 'b', 'c'] are the "strings", and 1, 2 are the "values". - */ - html(strings: TemplateStringsArray, ...values: unknown[]) { - if (this.lookupSet.has(strings)) { - return this.nativeHtml(strings, ...values); - } - - const key = strings.join('\0'); - const oldStrings = this.lookupMap.get(key); - - if (oldStrings === undefined) { - this.lookupSet.add(strings); - this.lookupMap.set(key, strings); - return this.nativeHtml(strings, ...values); - } - - if (oldStrings === strings) { - return this.nativeHtml(strings, ...values); - } - - // Without using HtmlPatcher html`` would be called with `strings`, - // which will be considered different, although actually being equal. - console.warn(`HtmlPatcher was required for '${key.substring(0, 100)}'.`); - this.reporter?.(key); - return this.nativeHtml(oldStrings, ...values); - } - - // Allows spying on calls in tests. - nativeHtml(strings: TemplateStringsArray, ...values: unknown[]) { - return html(strings, ...values); - } -}
diff --git a/polygerrit-ui/app/utils/lit-util_test.ts b/polygerrit-ui/app/utils/lit-util_test.ts deleted file mode 100644 index 17197f0..0000000 --- a/polygerrit-ui/app/utils/lit-util_test.ts +++ /dev/null
@@ -1,118 +0,0 @@ -/** - * @license - * Copyright 2020 Google LLC - * SPDX-License-Identifier: Apache-2.0 - */ -import {assert} from '@open-wc/testing'; -import '../test/common-test-setup'; -import {HtmlPatched} from './lit-util'; - -function tsa(strings: string[]): TemplateStringsArray { - return strings as unknown as TemplateStringsArray; -} - -suite('lit-util HtmlPatched tests', () => { - let patched: HtmlPatched; - let nativeHtmlSpy: sinon.SinonSpy; - let reporterSpy: sinon.SinonSpy; - - setup(async () => { - reporterSpy = sinon.spy(); - patched = new HtmlPatched(reporterSpy); - nativeHtmlSpy = sinon.spy(patched, 'nativeHtml'); - }); - - test('simple call', () => { - const instance1 = tsa(['1']); - patched.html(instance1, 'a value'); - assert.equal(nativeHtmlSpy.callCount, 1); - assert.equal(reporterSpy.callCount, 0); - assert.strictEqual(nativeHtmlSpy.getCalls()[0].args[0], instance1); - assert.strictEqual(nativeHtmlSpy.getCalls()[0].args[1], 'a value'); - }); - - test('two calls, same instance', () => { - const instance1 = tsa(['1']); - patched.html(instance1, 'a value'); - patched.html(instance1, 'a value'); - assert.equal(nativeHtmlSpy.callCount, 2); - assert.equal(reporterSpy.callCount, 0); - assert.strictEqual(nativeHtmlSpy.getCalls()[0].firstArg, instance1); - assert.strictEqual(nativeHtmlSpy.getCalls()[1].firstArg, instance1); - }); - - test('two calls, different strings', () => { - const instance1 = tsa(['1']); - const instance2 = tsa(['2']); - patched.html(instance1, 'a value'); - patched.html(instance2, 'a value'); - assert.equal(nativeHtmlSpy.callCount, 2); - assert.equal(reporterSpy.callCount, 0); - assert.strictEqual(nativeHtmlSpy.getCalls()[0].firstArg, instance1); - assert.strictEqual(nativeHtmlSpy.getCalls()[1].firstArg, instance2); - }); - - test('two calls, same strings, different instances', () => { - const instance1 = tsa(['1']); - const instance2 = tsa(['1']); - patched.html(instance1, 'a value'); - patched.html(instance2, 'a value'); - assert.equal(nativeHtmlSpy.callCount, 2); - assert.equal(reporterSpy.callCount, 1); - assert.strictEqual(nativeHtmlSpy.getCalls()[0].firstArg, instance1); - assert.strictEqual(nativeHtmlSpy.getCalls()[1].firstArg, instance1); - }); - - test('many calls', () => { - const instance1a = tsa(['1']); - const instance1b = tsa(['1']); - const instance1c = tsa(['1']); - const instance2a = tsa(['asdf', 'qwer']); - const instance2b = tsa(['asdf', 'qwer']); - const instance2c = tsa(['asdf', 'qwer']); - const instance3a = tsa(['asd', 'fqwer']); - const instance3b = tsa(['asd', 'fqwer']); - const instance3c = tsa(['asd', 'fqwer']); - - patched.html(instance1a, 'a value'); - patched.html(instance1a, 'a value'); - patched.html(instance1b, 'a value'); - patched.html(instance1b, 'a value'); - patched.html(instance1c, 'a value'); - patched.html(instance1c, 'a value'); - patched.html(instance2a, 'a value'); - patched.html(instance2a, 'a value'); - patched.html(instance2b, 'a value'); - patched.html(instance2b, 'a value'); - patched.html(instance2c, 'a value'); - patched.html(instance2c, 'a value'); - patched.html(instance3a, 'a value'); - patched.html(instance3a, 'a value'); - patched.html(instance3b, 'a value'); - patched.html(instance3b, 'a value'); - patched.html(instance3c, 'a value'); - patched.html(instance3c, 'a value'); - - assert.equal(nativeHtmlSpy.callCount, 18); - assert.equal(reporterSpy.callCount, 12); - - assert.strictEqual(nativeHtmlSpy.getCalls()[0].firstArg, instance1a); - assert.strictEqual(nativeHtmlSpy.getCalls()[1].firstArg, instance1a); - assert.strictEqual(nativeHtmlSpy.getCalls()[2].firstArg, instance1a); - assert.strictEqual(nativeHtmlSpy.getCalls()[3].firstArg, instance1a); - assert.strictEqual(nativeHtmlSpy.getCalls()[4].firstArg, instance1a); - assert.strictEqual(nativeHtmlSpy.getCalls()[5].firstArg, instance1a); - assert.strictEqual(nativeHtmlSpy.getCalls()[6].firstArg, instance2a); - assert.strictEqual(nativeHtmlSpy.getCalls()[7].firstArg, instance2a); - assert.strictEqual(nativeHtmlSpy.getCalls()[8].firstArg, instance2a); - assert.strictEqual(nativeHtmlSpy.getCalls()[9].firstArg, instance2a); - assert.strictEqual(nativeHtmlSpy.getCalls()[10].firstArg, instance2a); - assert.strictEqual(nativeHtmlSpy.getCalls()[11].firstArg, instance2a); - assert.strictEqual(nativeHtmlSpy.getCalls()[12].firstArg, instance3a); - assert.strictEqual(nativeHtmlSpy.getCalls()[13].firstArg, instance3a); - assert.strictEqual(nativeHtmlSpy.getCalls()[14].firstArg, instance3a); - assert.strictEqual(nativeHtmlSpy.getCalls()[15].firstArg, instance3a); - assert.strictEqual(nativeHtmlSpy.getCalls()[16].firstArg, instance3a); - assert.strictEqual(nativeHtmlSpy.getCalls()[17].firstArg, instance3a); - }); -});