From 0e913dcdbe87a28b557adf996cc0ebafa5a2927e Mon Sep 17 00:00:00 2001 From: lejunyang Date: Sat, 16 Nov 2024 17:54:54 +0800 Subject: [PATCH 1/5] fix(custom-element): fix parent, observer and exposed issues when remove element and append it back after fully unmounted (#12412) --- .../__tests__/customElement.spec.ts | 37 ++++++++++++++++ packages/runtime-dom/src/apiCustomElement.ts | 43 +++++++++++++------ 2 files changed, 67 insertions(+), 13 deletions(-) diff --git a/packages/runtime-dom/__tests__/customElement.spec.ts b/packages/runtime-dom/__tests__/customElement.spec.ts index df438d47eee..3b072782db0 100644 --- a/packages/runtime-dom/__tests__/customElement.spec.ts +++ b/packages/runtime-dom/__tests__/customElement.spec.ts @@ -145,6 +145,43 @@ describe('defineCustomElement', () => { expect(e._instance).toBeTruthy() expect(e.shadowRoot!.innerHTML).toBe('
hello
') }) + + // #12412 + test('remove element with child custom element and wait fully disconnected then insert', async () => { + const El = defineCustomElement({ + props: { + msg: String, + }, + setup(props, { expose }) { + expose({ + text: () => props.msg, + }) + provide('context', props) + const context = inject('context', {}) as typeof props + return () => context.msg || props.msg + }, + }) + customElements.define('my-el-remove-insert-expose', El) + container.innerHTML = `
` + const parent = container.children[0].children[0] as VueElement & { + text: () => string + } + const child = parent.children[0] as VueElement + parent.remove() + await nextTick() + await nextTick() // wait two ticks for disconnect + expect('text' in parent).toBe(false) + container.appendChild(parent) // should not throw Error + await nextTick() + expect(parent.text()).toBe('msg1') + expect(parent.shadowRoot!.textContent).toBe('msg1') + expect(child.shadowRoot!.textContent).toBe('msg1') + parent.setAttribute('msg', 'msg2') + await nextTick() + expect(parent.shadowRoot!.textContent).toBe('msg2') + await nextTick() + expect(child.shadowRoot!.textContent).toBe('msg2') + }) }) describe('props', () => { diff --git a/packages/runtime-dom/src/apiCustomElement.ts b/packages/runtime-dom/src/apiCustomElement.ts index aeeaeec9b9f..fb7fb98ecec 100644 --- a/packages/runtime-dom/src/apiCustomElement.ts +++ b/packages/runtime-dom/src/apiCustomElement.ts @@ -298,8 +298,9 @@ export class VueElement if (!this._instance) { if (this._resolved) { - this._setParent() - this._update() + // this element has been fully unmounted, should create observer again and re-mount + this._observe() + this._mount(this._def) } else { if (parent && parent._pendingResolve) { this._pendingResolve = parent._pendingResolve.then(() => { @@ -330,12 +331,31 @@ export class VueElement } // unmount this._app && this._app.unmount() - if (this._instance) this._instance.ce = undefined + if (this._instance) { + const exposed = this._instance.exposed + if (exposed) { + for (const key in exposed) { + delete this[key as keyof this] + } + } + this._instance.ce = undefined + } this._app = this._instance = null } }) } + private _observe() { + if (!this._ob) { + this._ob = new MutationObserver(mutations => { + for (const m of mutations) { + this._setAttr(m.attributeName!) + } + }) + } + this._ob.observe(this, { attributes: true }) + } + /** * resolve inner component definition (handle possible async component) */ @@ -350,13 +370,7 @@ export class VueElement } // watch future attr changes - this._ob = new MutationObserver(mutations => { - for (const m of mutations) { - this._setAttr(m.attributeName!) - } - }) - - this._ob.observe(this, { attributes: true }) + this._observe() const resolve = (def: InnerComponentDef, isAsync = false) => { this._resolved = true @@ -430,11 +444,14 @@ export class VueElement if (!hasOwn(this, key)) { // exposed properties are readonly Object.defineProperty(this, key, { + configurable: true, // should be configurable to allow deleting when disconnected // unwrap ref to be consistent with public instance behavior get: () => unref(exposed[key]), }) - } else if (__DEV__) { - warn(`Exposed property "${key}" already exists on custom element.`) + } else { + delete exposed[key] // delete it from exposed in case of deleting wrong exposed key when disconnected + if (__DEV__) + warn(`Exposed property "${key}" already exists on custom element.`) } } } @@ -514,7 +531,7 @@ export class VueElement } else if (!val) { this.removeAttribute(hyphenate(key)) } - ob && ob.observe(this, { attributes: true }) + this._observe() } } } From 77da780a1089b20f7772cd77a43b81d967c04242 Mon Sep 17 00:00:00 2001 From: lejunyang Date: Mon, 18 Nov 2024 22:49:02 +0800 Subject: [PATCH 2/5] fix(custom-element): should update when moving custom element to other element --- .../__tests__/customElement.spec.ts | 43 ++++++++++++------- packages/runtime-dom/src/apiCustomElement.ts | 39 ++++++++++------- 2 files changed, 50 insertions(+), 32 deletions(-) diff --git a/packages/runtime-dom/__tests__/customElement.spec.ts b/packages/runtime-dom/__tests__/customElement.spec.ts index 3b072782db0..976213f3783 100644 --- a/packages/runtime-dom/__tests__/customElement.spec.ts +++ b/packages/runtime-dom/__tests__/customElement.spec.ts @@ -147,22 +147,22 @@ describe('defineCustomElement', () => { }) // #12412 - test('remove element with child custom element and wait fully disconnected then insert', async () => { - const El = defineCustomElement({ - props: { - msg: String, - }, - setup(props, { expose }) { - expose({ - text: () => props.msg, - }) - provide('context', props) - const context = inject('context', {}) as typeof props - return () => context.msg || props.msg - }, - }) - customElements.define('my-el-remove-insert-expose', El) - container.innerHTML = `
` + const ContextEl = defineCustomElement({ + props: { + msg: String, + }, + setup(props, { expose }) { + expose({ + text: () => props.msg, + }) + provide('context', props) + const context = inject('context', {}) as typeof props + return () => context.msg || props.msg + }, + }) + customElements.define('my-context-el', ContextEl) + test('remove element with child custom element and wait fully disconnected then append and change attribute', async () => { + container.innerHTML = `
` const parent = container.children[0].children[0] as VueElement & { text: () => string } @@ -182,6 +182,17 @@ describe('defineCustomElement', () => { await nextTick() expect(child.shadowRoot!.textContent).toBe('msg2') }) + + test('move element to change parent and context', async () => { + container.innerHTML = `` + const first = container.children[0] as VueElement, + second = container.children[1] as VueElement + await nextTick() + expect(second.shadowRoot!.textContent).toBe('msg2') + first.append(second) + await nextTick() + expect(second.shadowRoot!.textContent).toBe('msg1') + }) }) describe('props', () => { diff --git a/packages/runtime-dom/src/apiCustomElement.ts b/packages/runtime-dom/src/apiCustomElement.ts index fb7fb98ecec..b85f7346b60 100644 --- a/packages/runtime-dom/src/apiCustomElement.ts +++ b/packages/runtime-dom/src/apiCustomElement.ts @@ -286,20 +286,24 @@ export class VueElement this._connected = true // locate nearest Vue custom element parent for provide/inject - let parent: Node | null = this + let parent: Node | null = this, + parentChanged = false while ( (parent = parent && (parent.parentNode || (parent as ShadowRoot).host)) ) { if (parent instanceof VueElement) { + parentChanged = parent !== this._parent this._parent = parent break } } - if (!this._instance) { + // unmount if parent changed and previously mounted + if (this._instance && parentChanged) this._unmount() + if (!this._instance || parentChanged) { if (this._resolved) { - // this element has been fully unmounted, should create observer again and re-mount - this._observe() + // no instance means observer is cleared, should observe again + if (!this._instance) this._observe() this._mount(this._def) } else { if (parent && parent._pendingResolve) { @@ -321,6 +325,20 @@ export class VueElement } } + private _unmount() { + this._app && this._app.unmount() + if (this._instance) { + const exposed = this._instance.exposed + if (exposed) { + for (const key in exposed) { + delete this[key as keyof this] + } + } + this._instance.ce = undefined + } + this._app = this._instance = null + } + disconnectedCallback(): void { this._connected = false nextTick(() => { @@ -329,18 +347,7 @@ export class VueElement this._ob.disconnect() this._ob = null } - // unmount - this._app && this._app.unmount() - if (this._instance) { - const exposed = this._instance.exposed - if (exposed) { - for (const key in exposed) { - delete this[key as keyof this] - } - } - this._instance.ce = undefined - } - this._app = this._instance = null + this._unmount() } }) } From f81a841011cd5d4637109bed69c5ebd6788c85bf Mon Sep 17 00:00:00 2001 From: lejunyang Date: Mon, 18 Nov 2024 23:38:25 +0800 Subject: [PATCH 3/5] fix(custom-element): reset resolved and parent in unmount to resoveDef again instead of only re-mount --- packages/runtime-dom/src/apiCustomElement.ts | 44 +++++++++----------- 1 file changed, 20 insertions(+), 24 deletions(-) diff --git a/packages/runtime-dom/src/apiCustomElement.ts b/packages/runtime-dom/src/apiCustomElement.ts index b85f7346b60..0e72fa0adb5 100644 --- a/packages/runtime-dom/src/apiCustomElement.ts +++ b/packages/runtime-dom/src/apiCustomElement.ts @@ -298,13 +298,12 @@ export class VueElement } } - // unmount if parent changed and previously mounted - if (this._instance && parentChanged) this._unmount() - if (!this._instance || parentChanged) { + // unmount if parent changed and previously mounted, should keep parent + if (this._instance && parentChanged) this._unmount(true) + if (!this._instance) { if (this._resolved) { - // no instance means observer is cleared, should observe again - if (!this._instance) this._observe() - this._mount(this._def) + this._setParent() + this._update() } else { if (parent && parent._pendingResolve) { this._pendingResolve = parent._pendingResolve.then(() => { @@ -325,7 +324,11 @@ export class VueElement } } - private _unmount() { + private _unmount(keepParent?: boolean) { + if (this._ob) { + this._ob.disconnect() + this._ob = null + } this._app && this._app.unmount() if (this._instance) { const exposed = this._instance.exposed @@ -337,32 +340,19 @@ export class VueElement this._instance.ce = undefined } this._app = this._instance = null + if (!keepParent) this._parent = undefined + this._resolved = false } disconnectedCallback(): void { this._connected = false nextTick(() => { if (!this._connected) { - if (this._ob) { - this._ob.disconnect() - this._ob = null - } this._unmount() } }) } - private _observe() { - if (!this._ob) { - this._ob = new MutationObserver(mutations => { - for (const m of mutations) { - this._setAttr(m.attributeName!) - } - }) - } - this._ob.observe(this, { attributes: true }) - } - /** * resolve inner component definition (handle possible async component) */ @@ -377,7 +367,13 @@ export class VueElement } // watch future attr changes - this._observe() + this._ob = new MutationObserver(mutations => { + for (const m of mutations) { + this._setAttr(m.attributeName!) + } + }) + + this._ob.observe(this, { attributes: true }) const resolve = (def: InnerComponentDef, isAsync = false) => { this._resolved = true @@ -538,7 +534,7 @@ export class VueElement } else if (!val) { this.removeAttribute(hyphenate(key)) } - this._observe() + ob && ob.observe(this, { attributes: true }) } } } From 53077a5d6c5ea7fbd4c2fbd13f0673c56a30f403 Mon Sep 17 00:00:00 2001 From: lejunyang Date: Tue, 19 Nov 2024 21:52:53 +0800 Subject: [PATCH 4/5] test: fix test case issue, outer element should be defined first --- packages/vue/__tests__/e2e/ssr-custom-element.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/vue/__tests__/e2e/ssr-custom-element.spec.ts b/packages/vue/__tests__/e2e/ssr-custom-element.spec.ts index c875f1bee69..62526beb20f 100644 --- a/packages/vue/__tests__/e2e/ssr-custom-element.spec.ts +++ b/packages/vue/__tests__/e2e/ssr-custom-element.spec.ts @@ -100,7 +100,6 @@ test('work with Teleport (shadowRoot: false)', async () => { }, { shadowRoot: false }, ) - customElements.define('my-y', Y) const P = defineSSRCustomElement( { render() { @@ -110,6 +109,7 @@ test('work with Teleport (shadowRoot: false)', async () => { { shadowRoot: false }, ) customElements.define('my-p', P) + customElements.define('my-y', Y) }) function getInnerHTML() { From 6b52371d822fce7146588a02a1907df7b9c22f11 Mon Sep 17 00:00:00 2001 From: lejunyang Date: Fri, 22 Nov 2024 23:34:56 +0800 Subject: [PATCH 5/5] fix(custom-element): change solution --- .../__tests__/customElement.spec.ts | 24 +++++++---- packages/runtime-dom/src/apiCustomElement.ts | 43 +++++++++++-------- 2 files changed, 41 insertions(+), 26 deletions(-) diff --git a/packages/runtime-dom/__tests__/customElement.spec.ts b/packages/runtime-dom/__tests__/customElement.spec.ts index 976213f3783..c0474768266 100644 --- a/packages/runtime-dom/__tests__/customElement.spec.ts +++ b/packages/runtime-dom/__tests__/customElement.spec.ts @@ -147,10 +147,12 @@ describe('defineCustomElement', () => { }) // #12412 + const contextElStyle = ':host { color: red }' const ContextEl = defineCustomElement({ props: { msg: String, }, + styles: [contextElStyle], setup(props, { expose }) { expose({ text: () => props.msg, @@ -171,27 +173,33 @@ describe('defineCustomElement', () => { await nextTick() await nextTick() // wait two ticks for disconnect expect('text' in parent).toBe(false) + expect(child.shadowRoot!.querySelectorAll('style').length).toBe(1) container.appendChild(parent) // should not throw Error await nextTick() expect(parent.text()).toBe('msg1') - expect(parent.shadowRoot!.textContent).toBe('msg1') - expect(child.shadowRoot!.textContent).toBe('msg1') + expect(parent.shadowRoot!.textContent).toBe(contextElStyle + 'msg1') + expect(child.shadowRoot!.textContent).toBe(contextElStyle + 'msg1') parent.setAttribute('msg', 'msg2') await nextTick() - expect(parent.shadowRoot!.textContent).toBe('msg2') + expect(parent.shadowRoot!.textContent).toBe(contextElStyle + 'msg2') await nextTick() - expect(child.shadowRoot!.textContent).toBe('msg2') + expect(child.shadowRoot!.textContent).toBe(contextElStyle + 'msg2') + expect(child.shadowRoot!.querySelectorAll('style').length).toBe(1) }) - test('move element to change parent and context', async () => { + test('move element to new parent', async () => { container.innerHTML = `` const first = container.children[0] as VueElement, - second = container.children[1] as VueElement + second = container.children[1] as VueElement & { text: () => string } await nextTick() - expect(second.shadowRoot!.textContent).toBe('msg2') + expect(second.shadowRoot!.textContent).toBe(contextElStyle + 'msg2') first.append(second) await nextTick() - expect(second.shadowRoot!.textContent).toBe('msg1') + expect(second.shadowRoot!.textContent).toBe(contextElStyle + 'msg1') + expect(second.shadowRoot!.querySelectorAll('style').length).toBe(1) + second.setAttribute('msg', 'msg3') + await nextTick() + expect(second.text()).toBe('msg3') }) }) diff --git a/packages/runtime-dom/src/apiCustomElement.ts b/packages/runtime-dom/src/apiCustomElement.ts index 0e72fa0adb5..b2b2d3f2011 100644 --- a/packages/runtime-dom/src/apiCustomElement.ts +++ b/packages/runtime-dom/src/apiCustomElement.ts @@ -298,12 +298,13 @@ export class VueElement } } - // unmount if parent changed and previously mounted, should keep parent + // unmount if parent changed and previously mounted, should keep parent and observer if (this._instance && parentChanged) this._unmount(true) - if (!this._instance) { + if (!this._instance || parentChanged) { if (this._resolved) { - this._setParent() - this._update() + // no instance means no observer + if (!this._instance) this._observe() + this._mount(this._def) } else { if (parent && parent._pendingResolve) { this._pendingResolve = parent._pendingResolve.then(() => { @@ -324,10 +325,13 @@ export class VueElement } } - private _unmount(keepParent?: boolean) { - if (this._ob) { - this._ob.disconnect() - this._ob = null + private _unmount(keepParentAndOb?: boolean) { + if (!keepParentAndOb) { + this._parent = undefined + if (this._ob) { + this._ob.disconnect() + this._ob = null + } } this._app && this._app.unmount() if (this._instance) { @@ -340,8 +344,6 @@ export class VueElement this._instance.ce = undefined } this._app = this._instance = null - if (!keepParent) this._parent = undefined - this._resolved = false } disconnectedCallback(): void { @@ -353,6 +355,17 @@ export class VueElement }) } + private _observe() { + if (!this._ob) { + this._ob = new MutationObserver(mutations => { + for (const m of mutations) { + this._setAttr(m.attributeName!) + } + }) + } + this._ob.observe(this, { attributes: true }) + } + /** * resolve inner component definition (handle possible async component) */ @@ -367,13 +380,7 @@ export class VueElement } // watch future attr changes - this._ob = new MutationObserver(mutations => { - for (const m of mutations) { - this._setAttr(m.attributeName!) - } - }) - - this._ob.observe(this, { attributes: true }) + this._observe() const resolve = (def: InnerComponentDef, isAsync = false) => { this._resolved = true @@ -534,7 +541,7 @@ export class VueElement } else if (!val) { this.removeAttribute(hyphenate(key)) } - ob && ob.observe(this, { attributes: true }) + this._observe() } } }