From 9d0dd41d7f885e1e58a573eba16c4c2d7b8c1850 Mon Sep 17 00:00:00 2001 From: Rene Date: Sun, 18 Jul 2021 23:16:37 +0200 Subject: [PATCH] simplify dom observer api and prevent memory leak --- .../src/observers/domObserver.ts | 60 +++++++++---------- packages/overlayscrollbars/src/options.ts | 4 +- .../observers/domObserver/index.browser.ts | 14 ++--- 3 files changed, 37 insertions(+), 41 deletions(-) diff --git a/packages/overlayscrollbars/src/observers/domObserver.ts b/packages/overlayscrollbars/src/observers/domObserver.ts index 824d374..fafb662 100644 --- a/packages/overlayscrollbars/src/observers/domObserver.ts +++ b/packages/overlayscrollbars/src/observers/domObserver.ts @@ -20,7 +20,7 @@ interface DOMTargetObserverOptions extends DOMObserverOptionsBase { _ignoreTargetChange?: DOMObserverIgnoreTargetChange; // a function which will prevent marking certain attributes as changed if it returns true } -type ContentChangeArrayItem = [string?, string?, boolean?] | null | undefined; +type ContentChangeArrayItem = [string?, string?] | null | undefined; export type DOMObserverEventContentChange = Array | false | null | undefined; @@ -56,29 +56,22 @@ export interface DOMObserver { * @param callback Callback which is called if one of the elements emits the corresponding event. * @returns A object which contains a set of helper functions to destroy and update the observation of elements. */ -const createEventContentChange = (target: Element, eventContentChange: DOMObserverEventContentChange, callback: (...args: any) => any) => { - let eventSet: Set<() => any> | undefined; - let onceSet: WeakMap | undefined; // use WeakMap instead of WeakSet because of IE11 support +const createEventContentChange = (target: Element, callback: (...args: any) => any, eventContentChange?: DOMObserverEventContentChange) => { + let map: WeakMap any]> | undefined; // weak map to prevent memory leak for detached elements let destroyed = false; const _destroy = () => { destroyed = true; - if (eventSet) { - eventSet.forEach((offFn) => { - offFn(); - }); - eventSet.clear(); - } }; const _updateElements = (getElements?: (selector: string) => Node[]) => { - if (eventSet && onceSet && eventContentChange) { - const eventElmList = eventContentChange.reduce>((arr, item) => { + if (eventContentChange) { + const eventElmList = eventContentChange.reduce>((arr, item) => { if (item) { const selector = item[0]; const eventNames = item[1]; const elements = eventNames && selector && (getElements ? getElements(selector) : find(selector, target)); if (elements && elements.length && eventNames && isString(eventNames)) { - push(arr, [elements, eventNames.trim(), !!item[2]], true); + push(arr, [elements, eventNames.trim()], true); } } return arr; @@ -87,31 +80,34 @@ const createEventContentChange = (target: Element, eventContentChange: DOMObserv each(eventElmList, (item) => each(item[0], (elm) => { const eventNames = item[1]; - const once = item[2]; + const entry = map!.get(elm); - if (once && !onceSet!.has(elm)) { - onceSet!.set(elm, 0); - on( - elm, - eventNames, - (event) => { - if (!destroyed) { - callback(event); - } - }, - { _once: once } - ); - } else { - eventSet!.add(on(elm, eventNames, callback)); + if (entry) { + const entryEventNames = entry[0]; + const entryOff = entry[1]; + + // in case an already registered element is registered again, unregister the previous events + if (entryEventNames === eventNames) { + entryOff(); + } } + + const off = on(elm, eventNames, (event: Event) => { + if (destroyed) { + off(); + map!.delete(elm); + } else { + callback(event); + } + }); + map!.set(elm, [eventNames, off]); }) ); } }; if (eventContentChange) { - eventSet = eventSet || new Set(); - onceSet = onceSet || new WeakMap(); + map = new WeakMap(); _updateElements(); } @@ -147,7 +143,6 @@ export const createDOMObserver = ( } = (options as DOMContentObserverOptions & DOMTargetObserverOptions) || {}; const { _destroy: destroyEventContentChange, _updateElements: updateEventContentChangeElements } = createEventContentChange( target, - isContentObserver && _eventContentChange, debounce( () => { if (isConnected) { @@ -155,7 +150,8 @@ export const createDOMObserver = ( } }, { _timeout: 33, _maxDelay: 99 } - ) + ), + _eventContentChange ); // MutationObserver diff --git a/packages/overlayscrollbars/src/options.ts b/packages/overlayscrollbars/src/options.ts index 043fbf9..5c77b14 100644 --- a/packages/overlayscrollbars/src/options.ts +++ b/packages/overlayscrollbars/src/options.ts @@ -34,7 +34,7 @@ export interface OSOptions { resize: ResizeBehavior; paddingAbsolute: boolean; updating: { - elementEvents: Array<[string, string, boolean?]> | null; + elementEvents: Array<[string, string]> | null; attributes: string[] | null; debounce: number | [number, number] | null; }; @@ -137,7 +137,7 @@ const defaultOptionsWithTemplate: OptionsWithOptionsTemplate = { resize: ['none', resizeAllowedValues], // none || both || horizontal || vertical || n || b || h || v paddingAbsolute: booleanFalseTemplate, // true || false updating: { - elementEvents: [[['img', 'load', true]], arrayNullValues], // array of tuples || null + elementEvents: [[['img', 'load']], arrayNullValues], // array of tuples || null attributes: [null, arrayNullValues], debounce: [ [0, 33], diff --git a/packages/overlayscrollbars/tests/browser/observers/domObserver/index.browser.ts b/packages/overlayscrollbars/tests/browser/observers/domObserver/index.browser.ts index 4774cef..f8164bf 100644 --- a/packages/overlayscrollbars/tests/browser/observers/domObserver/index.browser.ts +++ b/packages/overlayscrollbars/tests/browser/observers/domObserver/index.browser.ts @@ -58,7 +58,7 @@ const startBtn: HTMLButtonElement | null = document.querySelector('#start'); const hostSelector = '.host'; const ignorePrefix = 'ignore'; const attrs = ['id', 'class', 'style', 'open']; -const contentChangeArr: Array<[string?, string?, boolean?]> = [['img', 'load', true]]; +const contentChange: Array<[string?, string?]> = [['img', 'load']]; const domTargetObserverObservations: DOMTargetObserverResult[] = []; const domContentObserverObservations: DOMContentObserverResult[] = []; @@ -106,7 +106,7 @@ const targetDomObserver = createDOMObserver( } ); -const createContentDomOserver = (eventContentChange: Array<[string?, string?, boolean?] | null | undefined>) => { +const createContentDomOserver = (eventContentChange: Array<[string?, string?] | null | undefined>) => { return createDOMObserver( trargetContentElm!, true, @@ -147,7 +147,7 @@ const createContentDomOserver = (eventContentChange: Array<[string?, string?, bo ); }; -let contentDomObserver = createContentDomOserver(contentChangeArr); +let contentDomObserver = createContentDomOserver(contentChange); const getTotalObservations = () => domTargetObserverObservations.length + domContentObserverObservations.length; const getLast = (arr: T[], indexFromLast = 0): T => arr[arr.length - 1 - indexFromLast] || ({} as T); @@ -424,7 +424,7 @@ const addRemoveImgElmsFn = async () => { await addMultiple(); // remove load event from image test - const addChanged = async (newEventContentChange: Array<[string?, string?, boolean?] | null | undefined>) => { + const addChanged = async (newEventContentChange: Array<[string?, string?] | null | undefined>) => { contentDomObserver._destroy(); contentDomObserver = createContentDomOserver(newEventContentChange); @@ -446,7 +446,7 @@ const addRemoveImgElmsFn = async () => { }); contentDomObserver._destroy(); - contentDomObserver = createContentDomOserver(contentChangeArr); + contentDomObserver = createContentDomOserver(contentChange); }; await addChanged([['img', 'something'], ['img', 'something2'], ['img', ''], ['img', undefined], ['', ''], [undefined, undefined], null, undefined]); @@ -512,7 +512,7 @@ const addRemoveTransitionElmsFn = async () => { await startTransition(elm, expectTransitionEndContentChange && true); contentDomObserver._destroy(); - contentDomObserver = createContentDomOserver(contentChangeArr); + contentDomObserver = createContentDomOserver(contentChange); await startTransition(elm, expectTransitionEndContentChange && false); removeElements(elm); @@ -523,7 +523,7 @@ const addRemoveTransitionElmsFn = async () => { await add(false); contentDomObserver._destroy(); - contentDomObserver = createContentDomOserver(contentChangeArr.concat([['.transition', 'transitionend']])); + contentDomObserver = createContentDomOserver(contentChange.concat([['.transition', 'transitionend']])); await add(true); };