From d360da0e61e90c2ef71ba219256505e741429ca3 Mon Sep 17 00:00:00 2001 From: "ala'n (Alexey Stsefanovich)" Date: Thu, 11 Jul 2024 14:27:16 +0200 Subject: [PATCH 1/3] fix(esl-mixin-element): major fix for nested hierarchy mixin disconnection Closes: #2505 --- .../test/mixin.prototype.test.ts | 76 +++++++++ .../esl-mixin-element/test/mixin.test.ts | 156 +++++++++--------- .../ui/esl-mixin-registry.ts | 10 +- 3 files changed, 160 insertions(+), 82 deletions(-) create mode 100644 src/modules/esl-mixin-element/test/mixin.prototype.test.ts diff --git a/src/modules/esl-mixin-element/test/mixin.prototype.test.ts b/src/modules/esl-mixin-element/test/mixin.prototype.test.ts new file mode 100644 index 000000000..b0d8d2652 --- /dev/null +++ b/src/modules/esl-mixin-element/test/mixin.prototype.test.ts @@ -0,0 +1,76 @@ +import {ESLMixinElement} from '../ui/esl-mixin-element'; + +describe('ESLMixinElement.prototype implements all required ESLComponent API methods', () => { + class CTestMixin extends ESLMixinElement { + static override is = 'c-test'; + } + + let $el: CTestMixin; + + const $host = document.createElement('div'); + $host.toggleAttribute(CTestMixin.is, true); + + beforeAll(async () => { + document.body.appendChild($host); + CTestMixin.register(); + + await Promise.resolve(); + $el = CTestMixin.get($host) as CTestMixin; + }); + + describe('ESLMixinElement.prototype.$$attr persist and correct', () => { + const attrName = 'test-attr'; + const attrNameBool = 'test-attr-bool'; + + test('should return the initial attribute value', () => { + const val = $host.getAttribute(attrName); + expect($el.$$attr(attrName)).toBe(val); + }); + + test('should set the attribute value and return the previous value', () => { + expect($el.$$attr(attrName, 'test')).toBe(null); + expect($el.$$attr(attrName)).toBe('test'); + }); + + test('should set and return an empty string for boolean true', () => { + expect($el.$$attr(attrNameBool, true)).toBe(null); + expect($el.$$attr(attrNameBool)).toBe(''); + }); + + test('should set and return null for removed attribute', () => { + expect($el.$$attr(attrNameBool, false)).toBe(''); + expect($el.$$attr(attrNameBool)).toBe(null); + }); + }); + + describe('ESLMixinElement.prototype.$$cls persist and correct', () => { + test('should return false when class does not exist', () => expect($el.$$cls('a')).toBe(false)); + + test('should return true when class exists', () => { + $host.className = 'a b'; + expect($el.$$cls('a')).toBe(true); + expect($el.$$cls('a b')).toBe(true); + }); + + test('should add the class when setting to true', () => { + $host.className = ''; + $el.$$cls('a', true); + $el.$$cls('b', true); + expect($host.className).toBe('a b'); + }); + + test('should remove the class when setting to false', () => { + $host.className = 'a b'; + expect($el.$$cls('a b', false)).toBe(false); + expect($host.className).toBe(''); + }); + }); + + test('ESLMixinElement.prototype.$$fire persist and correct', () => { + const eventName = 'testevent'; + $host.dispatchEvent = jest.fn(); + $el.$$fire(eventName); + + expect($host.dispatchEvent).lastCalledWith(expect.objectContaining({type: eventName, cancelable: true, bubbles: true})); + }); +}); diff --git a/src/modules/esl-mixin-element/test/mixin.test.ts b/src/modules/esl-mixin-element/test/mixin.test.ts index 0adde94b6..2c1a97af2 100644 --- a/src/modules/esl-mixin-element/test/mixin.test.ts +++ b/src/modules/esl-mixin-element/test/mixin.test.ts @@ -2,11 +2,11 @@ import {ESLMixinElement} from '../core'; // TODO: exited with more cases describe('ESLMixinElement', () => { - describe('register', () => { - class TestMixin extends ESLMixinElement { - static override is = 'test-mixin'; - } + class TestMixin extends ESLMixinElement { + static override is = 'test-mixin'; + } + describe('ESLMixinElement.register behaves according specification', () => { test('Existing ESLMixinElement attribute handled on registration', () => { const $el = document.createElement('div'); $el.toggleAttribute(TestMixin.is, true); @@ -59,28 +59,6 @@ describe('ESLMixinElement', () => { expect(TestMixin.get($el)).toBeInstanceOf(ESLMixinElement); }); - test('ESLMixinElement can be resolved by mixin name', async () => { - const $el = document.createElement('div'); - $el.toggleAttribute(TestMixin.is, true); - document.body.appendChild($el); - TestMixin.register(); - await Promise.resolve(); // Wait for next microtask - expect(ESLMixinElement.get($el, TestMixin.is)).toBe(TestMixin.get($el)); - }); - - test('init child', async () => { - TestMixin.register(); - - const $root = document.createElement('div'); - document.body.appendChild($root); - const $el = document.createElement('div'); - $root.appendChild($el); - $el.toggleAttribute(TestMixin.is, true); - - await Promise.resolve(); // Wait for next microtask - expect(TestMixin.get($el)).toBeInstanceOf(ESLMixinElement); - }); - test('registration does not prevent appearing mixin from handling', async () => { class ATestMixin extends ESLMixinElement { static override is = 'a-test'; @@ -102,78 +80,96 @@ describe('ESLMixinElement', () => { }); }); - describe('ESLMixinElement prototype', () => { - class CTestMixin extends ESLMixinElement { - static override is = 'c-test'; - } + describe('ESLMixinElement.get api present and behaves according specification', () => { + test('ESLMixinElement can be resolved by mixin name', async () => { + const $el = document.createElement('div'); + $el.toggleAttribute(TestMixin.is, true); + document.body.appendChild($el); + TestMixin.register(); + await Promise.resolve(); // Wait for next microtask + expect(ESLMixinElement.get($el, TestMixin.is)).toBe(TestMixin.get($el)); + }); - let $el: CTestMixin; + // TODO: more cases for ESLMixinElement.get + }); - const $host = document.createElement('div'); - $host.toggleAttribute(CTestMixin.is, true); + describe('ESLMixinRegistry handles mixins deep in modified DOM hierarchy', () => { + const $root = document.createElement('div'); + const $el = document.createElement('div'); + $el.toggleAttribute(TestMixin.is, true); + $root.appendChild($el); beforeAll(async () => { - document.body.appendChild($host); - CTestMixin.register(); + TestMixin.register(); + document.body.appendChild($root); - await Promise.resolve(); - $el = CTestMixin.get($host) as CTestMixin; + await Promise.resolve(); // Wait for next microtask }); - describe('ESLMixinElement $attr', () => { - const attrName = 'test-attr'; - const attrNameBool = 'test-attr-bool'; + test('ESLMixinRegistry initialize mixins on child elements of added DOM part', () => { + expect(TestMixin.get($el)).toBeInstanceOf(ESLMixinElement); + }); + + test('ESLMixinRegistry disconnect all mixins on removed DOM part', async () => { + $root.remove(); + await Promise.resolve(); // Wait for next microtask + expect(TestMixin.get($el)).toBe(null); + }); + }); - test('should return the initial attribute value', () => { - const val = $host.getAttribute(attrName); - expect($el.$$attr(attrName)).toBe(val); - }); + describe('ESLMixinRegistry handles multiple mixins deep in modified DOM hierarchy', () => { + class TestMixin2 extends ESLMixinElement { + static instanceCounter = 0; + static override is = 'test-mixin-2'; - test('should set the attribute value and return the previous value', () => { - expect($el.$$attr(attrName, 'test')).toBe(null); - expect($el.$$attr(attrName)).toBe('test'); - }); + protected override connectedCallback(): void { + super.connectedCallback(); + TestMixin2.instanceCounter++; + } - test('should set and return an empty string for boolean true', () => { - expect($el.$$attr(attrNameBool, true)).toBe(null); - expect($el.$$attr(attrNameBool)).toBe(''); - }); + protected override disconnectedCallback(): void { + super.disconnectedCallback(); + TestMixin2.instanceCounter--; + } + } - test('should set and return null for removed attribute', () => { - expect($el.$$attr(attrNameBool, false)).toBe(''); - expect($el.$$attr(attrNameBool)).toBe(null); - }); - }); + const $root = document.createElement('div'); + const $el1 = document.createElement('div'); + const $el2 = document.createElement('div'); + $el1.toggleAttribute(TestMixin.is, true); + $el1.toggleAttribute(TestMixin2.is, true); + $el2.toggleAttribute(TestMixin2.is, true); + $el2.appendChild($el1); + $root.appendChild($el2); - describe('ESLMixinElement $cls', () => { - test('should return false when class does not exist', () => expect($el.$$cls('a')).toBe(false)); + beforeAll(async () => { + TestMixin.register(); + TestMixin2.register(); + document.body.appendChild($root); - test('should return true when class exists', () => { - $host.className = 'a b'; - expect($el.$$cls('a')).toBe(true); - expect($el.$$cls('a b')).toBe(true); - }); + await Promise.resolve(); // Wait for next microtask + }); - test('should add the class when setting to true', () => { - $host.className = ''; - $el.$$cls('a', true); - $el.$$cls('b', true); - expect($host.className).toBe('a b'); - }); + test('ESLMixinRegistry initialize all mixins on child elements of added DOM part', () => { + expect(TestMixin.get($el1)).toBeInstanceOf(ESLMixinElement); + expect(TestMixin2.get($el1)).toBeInstanceOf(ESLMixinElement); + expect(TestMixin2.get($el2)).toBeInstanceOf(ESLMixinElement); + }); - test('should remove the class when setting to false', () => { - $host.className = 'a b'; - expect($el.$$cls('a b', false)).toBe(false); - expect($host.className).toBe(''); - }); + test('Test mixin instance counter is correct after elements creation', () => { + expect(TestMixin2.instanceCounter).toBe(2); }); - test('ESLMixinElement $$fire', () => { - const eventName = 'testevent'; - $host.dispatchEvent = jest.fn(); - $el.$$fire(eventName); + test('ESLMixinRegistry disconnect all mixins on removed DOM part', async () => { + $root.remove(); + await Promise.resolve(); // Wait for next microtask + expect(TestMixin.get($el1)).toBe(null); + expect(TestMixin2.get($el1)).toBe(null); + expect(TestMixin2.get($el2)).toBe(null); + }); - expect($host.dispatchEvent).lastCalledWith(expect.objectContaining({type: eventName, cancelable: true, bubbles: true})); + test('Test mixin instance counter is correct after elements removal', () => { + expect(TestMixin2.instanceCounter).toBe(0); }); }); }); diff --git a/src/modules/esl-mixin-element/ui/esl-mixin-registry.ts b/src/modules/esl-mixin-element/ui/esl-mixin-registry.ts index 1206ca34b..f8ad37ae0 100644 --- a/src/modules/esl-mixin-element/ui/esl-mixin-registry.ts +++ b/src/modules/esl-mixin-element/ui/esl-mixin-registry.ts @@ -170,9 +170,15 @@ export class ESLMixinRegistry { delete store[mixin]; } - /** Destroys all mixins on the element */ - private static destroyAll(el: HTMLElement): void { + /** + * Destroys all mixins on the element + * @param el - host element to destroy all mixins + * @param deep - if true, will destroy all mixins on the subtree + */ + private static destroyAll(el: HTMLElement, deep = true): void { const store = (el as any)[STORE] as Record | undefined; store && Object.keys(store).forEach((name) => ESLMixinRegistry.destroy(el, name)); + if (!deep || !el.children || !el.children.length) return; + Array.prototype.forEach.call(el.children, (child: Element) => ESLMixinRegistry.destroyAll(child as HTMLElement)); } } From 7877eadf17047c7f2f9122c0fddf7d5fc3dbcf3a Mon Sep 17 00:00:00 2001 From: "ala'n (Alexey Stsefanovich)" Date: Thu, 11 Jul 2024 14:50:46 +0200 Subject: [PATCH 2/3] style: simplify `destroyAll` api due to private usage --- src/modules/esl-mixin-element/ui/esl-mixin-registry.ts | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/src/modules/esl-mixin-element/ui/esl-mixin-registry.ts b/src/modules/esl-mixin-element/ui/esl-mixin-registry.ts index f8ad37ae0..91e18ad12 100644 --- a/src/modules/esl-mixin-element/ui/esl-mixin-registry.ts +++ b/src/modules/esl-mixin-element/ui/esl-mixin-registry.ts @@ -170,15 +170,11 @@ export class ESLMixinRegistry { delete store[mixin]; } - /** - * Destroys all mixins on the element - * @param el - host element to destroy all mixins - * @param deep - if true, will destroy all mixins on the subtree - */ - private static destroyAll(el: HTMLElement, deep = true): void { + /** Destroys all mixins on the element and its subtree */ + private static destroyAll(el: HTMLElement): void { const store = (el as any)[STORE] as Record | undefined; store && Object.keys(store).forEach((name) => ESLMixinRegistry.destroy(el, name)); - if (!deep || !el.children || !el.children.length) return; + if (!el.children || !el.children.length) return; Array.prototype.forEach.call(el.children, (child: Element) => ESLMixinRegistry.destroyAll(child as HTMLElement)); } } From 3deab669e1af1619c3b1fb6d7d6580ef9c33784e Mon Sep 17 00:00:00 2001 From: "ala'n (Alexey Stsefanovich)" Date: Thu, 11 Jul 2024 14:56:49 +0200 Subject: [PATCH 3/3] style(esl-mixin-element): simplify subtree check conditions Co-authored-by: Anna --- src/modules/esl-mixin-element/ui/esl-mixin-registry.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/modules/esl-mixin-element/ui/esl-mixin-registry.ts b/src/modules/esl-mixin-element/ui/esl-mixin-registry.ts index 91e18ad12..60e30457d 100644 --- a/src/modules/esl-mixin-element/ui/esl-mixin-registry.ts +++ b/src/modules/esl-mixin-element/ui/esl-mixin-registry.ts @@ -69,7 +69,7 @@ export class ESLMixinRegistry { public invalidateRecursive(root: HTMLElement = document.body, name?: string): void { if (!root) return; name ? this.invalidate(root, name) : this.invalidateAll(root); - if (!root.children || !root.children.length) return; + if (!root.children?.length) return; Array.prototype.forEach.call(root.children, (child: Element) => this.invalidateRecursive(child as HTMLElement, name)); } @@ -174,7 +174,7 @@ export class ESLMixinRegistry { private static destroyAll(el: HTMLElement): void { const store = (el as any)[STORE] as Record | undefined; store && Object.keys(store).forEach((name) => ESLMixinRegistry.destroy(el, name)); - if (!el.children || !el.children.length) return; + if (!el.children?.length) return; Array.prototype.forEach.call(el.children, (child: Element) => ESLMixinRegistry.destroyAll(child as HTMLElement)); } }