From a075de8ab77493339cf9e5708c2779d693eccd31 Mon Sep 17 00:00:00 2001 From: Seth Silesky <5115498+silesky@users.noreply.github.com> Date: Tue, 17 Dec 2024 16:04:56 -0600 Subject: [PATCH] add dedupe logic --- .../__tests__/mutation-observer.test.ts | 203 ++++++++++++++++++ .../signal-generators/dom-gen/change-gen.ts | 17 +- .../dom-gen/mutation-observer.test.ts | 108 ---------- .../dom-gen/mutation-observer.ts | 73 +++++-- 4 files changed, 256 insertions(+), 145 deletions(-) create mode 100644 packages/signals/signals/src/core/signal-generators/dom-gen/__tests__/mutation-observer.test.ts delete mode 100644 packages/signals/signals/src/core/signal-generators/dom-gen/mutation-observer.test.ts diff --git a/packages/signals/signals/src/core/signal-generators/dom-gen/__tests__/mutation-observer.test.ts b/packages/signals/signals/src/core/signal-generators/dom-gen/__tests__/mutation-observer.test.ts new file mode 100644 index 000000000..ebdc561d4 --- /dev/null +++ b/packages/signals/signals/src/core/signal-generators/dom-gen/__tests__/mutation-observer.test.ts @@ -0,0 +1,203 @@ +/* eslint-disable jest/no-done-callback */ +import { sleep } from '@segment/analytics-core' +import { + MutationObservable, + MutationObservableSettings, + MutationObservableSubscriber, +} from '../mutation-observer' + +describe('MutationObservable', () => { + let mutationObservable: MutationObservable + let testButton: HTMLButtonElement + let testInput: HTMLInputElement + const subscribeFn = jest.fn() as jest.Mock<MutationObservableSubscriber> + beforeEach(() => { + document.body.innerHTML = + '<div id="test-element" role="button" aria-pressed="false"></div>' + + '<input id="test-input" value="" aria-foo="123" />' + testButton = document.getElementById('test-element') as HTMLButtonElement + testInput = document.getElementById('test-input') as HTMLInputElement + }) + + afterEach(() => { + mutationObservable.cleanup() + }) + + it('should capture attribute changes', async () => { + mutationObservable = new MutationObservable( + new MutationObservableSettings({ + observedRoles: () => ['button'], + observedAttributes: () => ['aria-pressed'], + debounceMs: 500, + }) + ) + + mutationObservable.subscribe(subscribeFn) + testButton.setAttribute('aria-pressed', 'true') + await sleep(0) + + expect(subscribeFn).toHaveBeenCalledTimes(1) + expect(subscribeFn).toHaveBeenCalledWith({ + element: testButton, + attributes: { 'aria-pressed': 'true' }, + }) + }) + + it('should capture multiple attribute changes', async () => { + mutationObservable = new MutationObservable( + new MutationObservableSettings({ + observedRoles: () => ['button'], + observedAttributes: () => ['aria-pressed'], + debounceMs: 500, + }) + ) + + mutationObservable.subscribe(subscribeFn) + testButton.setAttribute('aria-pressed', 'true') + await sleep(0) + testButton.setAttribute('aria-pressed', 'false') + await sleep(0) + + expect(subscribeFn).toHaveBeenCalledTimes(2) + expect(subscribeFn).toHaveBeenNthCalledWith(1, { + element: testButton, + attributes: { 'aria-pressed': 'true' }, + }) + expect(subscribeFn).toHaveBeenNthCalledWith(2, { + element: testButton, + attributes: { 'aria-pressed': 'false' }, + }) + }) + + it('should debounce attribute changes if they occur in text inputs', async () => { + mutationObservable = new MutationObservable( + new MutationObservableSettings({ + debounceMs: 100, + }) + ) + mutationObservable.subscribe(subscribeFn) + testInput.setAttribute('value', 'hello') + await sleep(0) + testInput.setAttribute('value', 'hello wor') + await sleep(0) + testInput.setAttribute('value', 'hello world') + + await sleep(200) + expect(subscribeFn).toHaveBeenCalledTimes(1) + expect(subscribeFn).toHaveBeenCalledWith({ + element: testInput, + attributes: { value: 'hello world' }, + }) + }) + + it('should handle multiple attributes changeing', async () => { + mutationObservable = new MutationObservable( + new MutationObservableSettings({ + debounceMs: 100, + observedAttributes: (roles) => [...roles, 'aria-foo'], + }) + ) + mutationObservable.subscribe(subscribeFn) + testInput.setAttribute('value', 'hello') + testInput.setAttribute('aria-foo', 'bar') + await sleep(200) + + expect(subscribeFn).toHaveBeenCalledTimes(1) + expect(subscribeFn).toHaveBeenCalledWith({ + element: testInput, + attributes: { value: 'hello', 'aria-foo': 'bar' }, + }) + }) + + it('should debounce if happening in the same tick', async () => { + mutationObservable = new MutationObservable( + new MutationObservableSettings({ + debounceMs: 50, + }) + ) + mutationObservable.subscribe(subscribeFn) + testInput.setAttribute('value', 'hello') + testInput.setAttribute('value', 'hello wor') + testInput.setAttribute('value', 'hello world') + await sleep(100) + + expect(subscribeFn).toHaveBeenCalledTimes(1) + expect(subscribeFn).toHaveBeenCalledWith({ + element: testInput, + attributes: { value: 'hello world' }, + }) + }) + + it('should not emit duplicate events', async () => { + mutationObservable = new MutationObservable( + new MutationObservableSettings({ + observedRoles: () => ['button'], + observedAttributes: () => ['aria-pressed'], + debounceMs: 0, + }) + ) + + mutationObservable.subscribe(subscribeFn) + testButton.setAttribute('aria-pressed', 'true') + await sleep(0) + testButton.setAttribute('aria-pressed', 'true') + await sleep(0) + + expect(subscribeFn).toHaveBeenCalledTimes(1) + expect(subscribeFn).toHaveBeenCalledWith({ + element: testButton, + attributes: { 'aria-pressed': 'true' }, + }) + }) + + it('should not emit duplicate events if overlapping', async () => { + mutationObservable = new MutationObservable( + new MutationObservableSettings({ + observedRoles: () => ['button'], + observedAttributes: () => ['aria-pressed', 'aria-foo'], + debounceMs: 0, + }) + ) + + mutationObservable.subscribe(subscribeFn) + testButton.setAttribute('aria-pressed', 'true') + testButton.setAttribute('aria-foo', 'bar') + await sleep(0) + + testButton.setAttribute('aria-pressed', 'false') + await sleep(50) + + testButton.setAttribute('aria-pressed', 'false') + await sleep(50) + + testButton.setAttribute('aria-foo', 'bar') + await sleep(50) + + expect(subscribeFn).toHaveBeenNthCalledWith(1, { + element: testButton, + attributes: { 'aria-pressed': 'true', 'aria-foo': 'bar' }, + }) + + expect(subscribeFn).toHaveBeenNthCalledWith(2, { + element: testButton, + attributes: { 'aria-pressed': 'false' }, + }) + expect(subscribeFn).toHaveBeenCalledTimes(2) + }) + + it('should not emit event for aria-selected=false', (done) => { + mutationObservable = new MutationObservable( + new MutationObservableSettings({ + observedRoles: () => ['button'], + observedAttributes: () => ['aria-selected'], + }) + ) + + mutationObservable.subscribe(() => { + done.fail('Should not emit event for aria-selected=false') + }) + + testButton.setAttribute('aria-selected', 'false') + setTimeout(done, 1000) // Wait to ensure no event is emitted + }) +}) diff --git a/packages/signals/signals/src/core/signal-generators/dom-gen/change-gen.ts b/packages/signals/signals/src/core/signal-generators/dom-gen/change-gen.ts index 39c3666cd..7a9c2b7f5 100644 --- a/packages/signals/signals/src/core/signal-generators/dom-gen/change-gen.ts +++ b/packages/signals/signals/src/core/signal-generators/dom-gen/change-gen.ts @@ -25,21 +25,6 @@ export class MutationChangeGenerator implements SignalGenerator { } register(emitter: SignalEmitter) { - type NormalizedAttributes = { [attributeName: string]: string | null } - const normalizeAttributes = ( - attributeMutation: AttributeChangedEvent - ): NormalizedAttributes => { - const attributes = - attributeMutation.attributes.reduce<NormalizedAttributes>( - (acc, { attributeName, newValue }) => { - acc[attributeName] = newValue - return acc - }, - {} - ) - return attributes - } - const callback = (ev: AttributeChangedEvent) => { const target = ev.element as HTMLElement | null if (!target || shouldIgnoreElement(target)) { @@ -51,7 +36,7 @@ export class MutationChangeGenerator implements SignalGenerator { eventType: 'change', target: el, listener: 'mutation', - change: normalizeAttributes(ev), + change: ev.attributes, }) ) } diff --git a/packages/signals/signals/src/core/signal-generators/dom-gen/mutation-observer.test.ts b/packages/signals/signals/src/core/signal-generators/dom-gen/mutation-observer.test.ts deleted file mode 100644 index fb2eea073..000000000 --- a/packages/signals/signals/src/core/signal-generators/dom-gen/mutation-observer.test.ts +++ /dev/null @@ -1,108 +0,0 @@ -/* eslint-disable jest/no-done-callback */ -import { sleep } from '@segment/analytics-core' -import { - MutationObservable, - MutationObservableSettings, - MutationObservableSubscriber, -} from './mutation-observer' - -describe('MutationObservable', () => { - let mutationObservable: MutationObservable - let testButton: HTMLButtonElement - let testInput: HTMLInputElement - const subscribeFn = jest.fn() as jest.Mock<MutationObservableSubscriber> - beforeEach(() => { - document.body.innerHTML = - '<div id="test-element" role="button" aria-pressed="false"></div>' + - '<input id="test-input" />' - testButton = document.getElementById('test-element') as HTMLButtonElement - testInput = document.getElementById('test-input') as HTMLInputElement - }) - - afterEach(() => { - mutationObservable.cleanup() - }) - - it('should capture attribute changes', async () => { - mutationObservable = new MutationObservable( - new MutationObservableSettings({ - observedRoles: () => ['button'], - observedAttributes: () => ['aria-pressed'], - debounceMs: 500, - }) - ) - - mutationObservable.subscribe(subscribeFn) - testButton.setAttribute('aria-pressed', 'true') - await sleep(0) - - expect(subscribeFn).toHaveBeenCalledTimes(1) - expect(subscribeFn).toHaveBeenCalledWith({ - element: testButton, - attributes: [{ attributeName: 'aria-pressed', newValue: 'true' }], - }) - }) - - it('should capture multiple attribute changes', async () => { - mutationObservable = new MutationObservable( - new MutationObservableSettings({ - observedRoles: () => ['button'], - observedAttributes: () => ['aria-pressed'], - debounceMs: 500, - }) - ) - - mutationObservable.subscribe(subscribeFn) - testButton.setAttribute('aria-pressed', 'true') - await sleep(0) - testButton.setAttribute('aria-pressed', 'false') - await sleep(0) - - expect(subscribeFn).toHaveBeenCalledTimes(2) - expect(subscribeFn).toHaveBeenNthCalledWith(1, { - element: testButton, - attributes: [{ attributeName: 'aria-pressed', newValue: 'true' }], - }) - expect(subscribeFn).toHaveBeenNthCalledWith(2, { - element: testButton, - attributes: [{ attributeName: 'aria-pressed', newValue: 'false' }], - }) - }) - - it('should debounce attribute changes if they occur in text inputs', async () => { - mutationObservable = new MutationObservable( - new MutationObservableSettings({ - debounceMs: 100, - }) - ) - mutationObservable.subscribe(subscribeFn) - testInput.setAttribute('value', 'hello') - await sleep(0) - testInput.setAttribute('value', 'hello wor') - await sleep(0) - testInput.setAttribute('value', 'hello world') - await sleep(200) - - expect(subscribeFn).toHaveBeenCalledTimes(1) - expect(subscribeFn).toHaveBeenCalledWith({ - element: testInput, - attributes: [{ attributeName: 'value', newValue: 'hello world' }], - }) - }) - - it('should not emit event for aria-selected=false', (done) => { - mutationObservable = new MutationObservable( - new MutationObservableSettings({ - observedRoles: () => ['button'], - observedAttributes: () => ['aria-selected'], - }) - ) - - mutationObservable.subscribe(() => { - done.fail('Should not emit event for aria-selected=false') - }) - - testButton.setAttribute('aria-selected', 'false') - setTimeout(done, 1000) // Wait to ensure no event is emitted - }) -}) diff --git a/packages/signals/signals/src/core/signal-generators/dom-gen/mutation-observer.ts b/packages/signals/signals/src/core/signal-generators/dom-gen/mutation-observer.ts index 54e0215a8..fc0e12486 100644 --- a/packages/signals/signals/src/core/signal-generators/dom-gen/mutation-observer.ts +++ b/packages/signals/signals/src/core/signal-generators/dom-gen/mutation-observer.ts @@ -32,13 +32,18 @@ const DEFAULT_OBSERVED_ROLES = [ 'treeitem', ] -type AttributeMutation = { - attributeName: string - newValue: string | null +const partialMatch = <Obj extends Record<string, any>>( + a: Partial<Obj>, + b: Obj +): boolean => { + return Object.keys(a).every( + (key) => a[key as keyof Obj] === b[key as keyof Obj] + ) } +type AttributeMutations = { [attributeName: string]: string | null } export type AttributeChangedEvent = { element: HTMLElement - attributes: AttributeMutation[] + attributes: AttributeMutations } export interface MutationObservableSettingsConfig { @@ -53,7 +58,7 @@ export interface MutationObservableSettingsConfig { export class MutationObservableSettings { pollIntervalMs: number - debounceTextInputMs: number + debounceMs: number emitInputStrategy: 'debounce-only' | 'blur' extraSelectors: string[] observedRoles: string[] @@ -72,12 +77,10 @@ export class MutationObservableSettings { if (pollIntervalMs < 300) { throw new Error('Poll interval must be at least 300ms') } - if (debounceMs < 100) { - throw new Error('Debounce must be at least 100ms') - } + this.emitInputStrategy = emitInputStrategy this.pollIntervalMs = pollIntervalMs - this.debounceTextInputMs = debounceMs + this.debounceMs = debounceMs this.extraSelectors = extraSelectors this.observedRoles = observedRoles @@ -146,6 +149,7 @@ export class MutationObservable { // Track observed elements to avoid duplicate observers // WeakSet is used here to allow garbage collection of elements that are no longer in the DOM private observedElements = new WeakSet() + private prevMutationsCache = new WeakMap<HTMLElement, AttributeMutations>() private emitter = new ElementChangedEmitter() private listeners = new Set<MutationObservableSubscriber>() @@ -173,9 +177,12 @@ export class MutationObservable { ) } - private shouldEmitEvent(mut: AttributeMutation): boolean { + private shouldEmitEvent( + attributeName: string, + newValue: string | null + ): boolean { // Filter out aria-selected events where the new value is false, since there will always be another selected value -- otherwise, checked would/should be used - if (mut.attributeName === 'aria-selected' && mut.newValue === 'false') { + if (attributeName === 'aria-selected' && newValue === 'false') { return false } return true @@ -188,13 +195,13 @@ export class MutationObservable { attributes: string[], emitter: ElementChangedEmitter ) { - const _emitAttributeMutationEvent = (attributes: AttributeMutation[]) => { + const _emitAttributeMutationEvent = (attributes: AttributeMutations) => { emitter.emit('attributeChanged', { element, attributes, }) } - const addOnBlurListener = (attributeMutations: AttributeMutation[]) => + const addOnBlurListener = (attributeMutations: AttributeMutations) => this.experimentalOnChangeAdapter.onBlur(element, () => _emitAttributeMutationEvent(attributeMutations) ) @@ -210,14 +217,14 @@ export class MutationObservable { ? debounceWithKey( emit, // debounce based on the attribute names, so that we can debounce all changes to a single attribute. e.g if attribute "value" changes, that gets debounced, but if another attribute changes, that gets debounced separately - (m) => Object.keys(m.map((m) => m.attributeName)).sort(), - this.settings.debounceTextInputMs + (m) => Object.keys(m).sort(), + this.settings.debounceMs ) : _emitAttributeMutationEvent // any call to setAttribute triggers a mutation event const cb: MutationCallback = (mutationsList) => { - const attributeMutations = mutationsList + const mutations: AttributeMutations = mutationsList .filter((m) => m.type === 'attributes') .map((m) => { const attributeName = m.attributeName @@ -226,10 +233,10 @@ export class MutationObservable { return const newValue = target.getAttribute(attributeName) - const v: AttributeMutation = { + const v = { attributeName, newValue: newValue, - } + } as const logger.debug('Attribute mutation', { newValue, oldValue: m.oldValue, @@ -238,11 +245,35 @@ export class MutationObservable { return v }) .filter(exists) - .filter((event) => this.shouldEmitEvent(event)) + .filter((event) => + this.shouldEmitEvent(event.attributeName, event.newValue) + ) + .reduce<AttributeMutations>((acc, mut) => { + acc[mut.attributeName] = mut.newValue + return acc + }, {}) - if (attributeMutations.length) { - _emitAttributeMutationEventDebounced(attributeMutations) + const isEmpty = Object.keys(mutations).length > 0 + if (!isEmpty) { + return } + + // only emit if there are actual change to an attribute. + // in mutationObserver, setAttribute('value', ''), setAttribute('value', '') will both trigger a mutation event + // if the value is the same as the last one emitted from a given element, we don't want to emit it again + const prevMutations = this.prevMutationsCache.get(element) + if (prevMutations) { + const hasActuallyChanged = !partialMatch(mutations, prevMutations) + if (!hasActuallyChanged) { + return + } + } + this.prevMutationsCache.set(element, { + ...prevMutations, + ...mutations, + }) + + _emitAttributeMutationEventDebounced(mutations) } const observer = new MutationObserver(cb)