From c35af6e5ae1e40b2a21f6f56988f4e3d59609819 Mon Sep 17 00:00:00 2001 From: David Turissini Date: Wed, 7 Nov 2018 10:04:16 -0800 Subject: [PATCH 1/5] fix(engine): marking dynamic nodes as dynamic --- packages/lwc-engine/src/framework/api.ts | 19 ++++++++----- packages/lwc-engine/src/framework/patch.ts | 8 +++--- .../duplicate-text-rendering.html | 15 +++++++++++ .../duplicate-text-rendering.js | 27 +++++++++++++++++++ 4 files changed, 59 insertions(+), 10 deletions(-) create mode 100644 packages/lwc-integration/src/components/rendering/test-duplicate-text-rendering/integration/duplicate-text-rendering/duplicate-text-rendering.html create mode 100644 packages/lwc-integration/src/components/rendering/test-duplicate-text-rendering/integration/duplicate-text-rendering/duplicate-text-rendering.js diff --git a/packages/lwc-engine/src/framework/api.ts b/packages/lwc-engine/src/framework/api.ts index 05cb95d4d9..8d39fde6ba 100644 --- a/packages/lwc-engine/src/framework/api.ts +++ b/packages/lwc-engine/src/framework/api.ts @@ -462,14 +462,15 @@ export function f(items: any[]): any[] { const item = items[j]; if (isArray(item)) { ArrayPush.apply(flattened, item); - // iteration mark propagation so the flattened structure can - // be diffed correctly. - if (hasDynamicChildren(item)) { - markAsDynamicChildren(flattened); - } } else { ArrayPush.call(flattened, item); } + + // iteration mark propagation so the flattened structure can + // be diffed correctly. + if (hasDynamicChildren(item)) { + markAsDynamicChildren(flattened); + } } return flattened; } @@ -515,7 +516,13 @@ export function d(value: any): VNode | null { if (value == null) { return null; } - return t(value); + const vtext = t(value); + + // The "children" here is really just a text value + // but the value can change between renders + // so we have to mark as dynamic + markAsDynamicChildren(vtext); + return vtext; } // [b]ind function diff --git a/packages/lwc-engine/src/framework/patch.ts b/packages/lwc-engine/src/framework/patch.ts index d3e11cb6e7..879f1bc71f 100644 --- a/packages/lwc-engine/src/framework/patch.ts +++ b/packages/lwc-engine/src/framework/patch.ts @@ -1,4 +1,4 @@ -import { VNodes } from "../3rdparty/snabbdom/types"; +import { VNodes, VText } from "../3rdparty/snabbdom/types"; import { patchEvent } from "../faux-shadow/faux"; import { tagNameGetter } from "../env/element"; import { updateDynamicChildren, updateStaticChildren } from "../3rdparty/snabbdom/snabbdom"; @@ -8,15 +8,15 @@ import { HTMLElementConstructor } from "./base-bridge-element"; import { PatchedElement, PatchedSlotElement, PatchedNode, PatchedIframeElement, PatchedCustomElement } from '../faux-shadow/faux'; // Using a WeakMap instead of a WeakSet because this one works in IE11 :( -const FromIteration: WeakMap = new WeakMap(); +const FromIteration: WeakMap = new WeakMap(); // dynamic children means it was generated by an iteration // in a template, and will require a more complex diffing algo. -export function markAsDynamicChildren(children: VNodes) { +export function markAsDynamicChildren(children: VNodes | VText) { FromIteration.set(children, 1); } -export function hasDynamicChildren(children: VNodes): boolean { +export function hasDynamicChildren(children: VNodes | VText): boolean { return FromIteration.has(children); } diff --git a/packages/lwc-integration/src/components/rendering/test-duplicate-text-rendering/integration/duplicate-text-rendering/duplicate-text-rendering.html b/packages/lwc-integration/src/components/rendering/test-duplicate-text-rendering/integration/duplicate-text-rendering/duplicate-text-rendering.html new file mode 100644 index 0000000000..80ded3ad03 --- /dev/null +++ b/packages/lwc-integration/src/components/rendering/test-duplicate-text-rendering/integration/duplicate-text-rendering/duplicate-text-rendering.html @@ -0,0 +1,15 @@ + diff --git a/packages/lwc-integration/src/components/rendering/test-duplicate-text-rendering/integration/duplicate-text-rendering/duplicate-text-rendering.js b/packages/lwc-integration/src/components/rendering/test-duplicate-text-rendering/integration/duplicate-text-rendering/duplicate-text-rendering.js new file mode 100644 index 0000000000..292a9dfcaa --- /dev/null +++ b/packages/lwc-integration/src/components/rendering/test-duplicate-text-rendering/integration/duplicate-text-rendering/duplicate-text-rendering.js @@ -0,0 +1,27 @@ +import { LightningElement, track } from 'lwc'; + +export default class App extends LightningElement { + @track text = [{ + text: 'a', + highlight: false, + }] + + get hasParts() { + return Array.isArray(this.text) && this.text.length > 0; + } + + get highlight() { + return false + } + + get firstPart() { + return this.text[0] + } + + connectedCallback() { + this.addEventListener('click', () => { + this.text = 'b' + }) + } + +} From d195bb81794e6c860837aef2b24d89ce7709f1a7 Mon Sep 17 00:00:00 2001 From: David Turissini Date: Wed, 7 Nov 2018 10:11:18 -0800 Subject: [PATCH 2/5] fix(engine): adding spec file --- .../duplicate-text-rendering.spec.js | 13 +++++++++++++ 1 file changed, 13 insertions(+) create mode 100644 packages/lwc-integration/src/components/rendering/test-duplicate-text-rendering/duplicate-text-rendering.spec.js diff --git a/packages/lwc-integration/src/components/rendering/test-duplicate-text-rendering/duplicate-text-rendering.spec.js b/packages/lwc-integration/src/components/rendering/test-duplicate-text-rendering/duplicate-text-rendering.spec.js new file mode 100644 index 0000000000..c7dfbdb6d2 --- /dev/null +++ b/packages/lwc-integration/src/components/rendering/test-duplicate-text-rendering/duplicate-text-rendering.spec.js @@ -0,0 +1,13 @@ +const assert = require('assert'); +describe('Dynamic text nodes rendering duplicate text', () => { + const URL = 'http://localhost:4567/duplicate-text-rendering'; + + before(() => { + browser.url(URL); + }); + + it('should not render duplicate text', function () { + browser.click('integration-duplicate-text-rendering'); + assert.deepEqual(browser.getText('integration-duplicate-text-rendering'), 'b'); + }); +}); From a0b76e23fe69a62310e5806ff1ee20ee5fd90d9a Mon Sep 17 00:00:00 2001 From: David Turissini Date: Wed, 7 Nov 2018 10:37:20 -0800 Subject: [PATCH 3/5] fix(engine): moving dynamic children marker --- packages/lwc-engine/src/framework/api.ts | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/packages/lwc-engine/src/framework/api.ts b/packages/lwc-engine/src/framework/api.ts index 8d39fde6ba..231d89fd80 100644 --- a/packages/lwc-engine/src/framework/api.ts +++ b/packages/lwc-engine/src/framework/api.ts @@ -458,6 +458,11 @@ export function f(items: any[]): any[] { } const len = items.length; const flattened: VNodes = []; + + // all flattened nodes should be marked as dynamic + // TODO: compiler should give us some sort of indicator + // to describe whether a vnode is dynamic or not + markAsDynamicChildren(flattened); for (let j = 0; j < len; j += 1) { const item = items[j]; if (isArray(item)) { @@ -465,12 +470,6 @@ export function f(items: any[]): any[] { } else { ArrayPush.call(flattened, item); } - - // iteration mark propagation so the flattened structure can - // be diffed correctly. - if (hasDynamicChildren(item)) { - markAsDynamicChildren(flattened); - } } return flattened; } From b7829b5c1f3c56a1b519a453e36cb26fad0242e2 Mon Sep 17 00:00:00 2001 From: David Turissini Date: Wed, 7 Nov 2018 10:39:27 -0800 Subject: [PATCH 4/5] fix(engine): reverting types --- packages/lwc-engine/src/framework/api.ts | 10 ++-------- packages/lwc-engine/src/framework/patch.ts | 8 ++++---- 2 files changed, 6 insertions(+), 12 deletions(-) diff --git a/packages/lwc-engine/src/framework/api.ts b/packages/lwc-engine/src/framework/api.ts index 231d89fd80..3c4aedc1e7 100644 --- a/packages/lwc-engine/src/framework/api.ts +++ b/packages/lwc-engine/src/framework/api.ts @@ -27,7 +27,7 @@ import { createTextHook, createCommentHook, } from "./hooks"; -import { markAsDynamicChildren, hasDynamicChildren, patchEvent } from "./patch"; +import { markAsDynamicChildren, patchEvent } from "./patch"; import { isNativeShadowRootAvailable, } from "../env/dom"; @@ -515,13 +515,7 @@ export function d(value: any): VNode | null { if (value == null) { return null; } - const vtext = t(value); - - // The "children" here is really just a text value - // but the value can change between renders - // so we have to mark as dynamic - markAsDynamicChildren(vtext); - return vtext; + return t(value); } // [b]ind function diff --git a/packages/lwc-engine/src/framework/patch.ts b/packages/lwc-engine/src/framework/patch.ts index 879f1bc71f..d3e11cb6e7 100644 --- a/packages/lwc-engine/src/framework/patch.ts +++ b/packages/lwc-engine/src/framework/patch.ts @@ -1,4 +1,4 @@ -import { VNodes, VText } from "../3rdparty/snabbdom/types"; +import { VNodes } from "../3rdparty/snabbdom/types"; import { patchEvent } from "../faux-shadow/faux"; import { tagNameGetter } from "../env/element"; import { updateDynamicChildren, updateStaticChildren } from "../3rdparty/snabbdom/snabbdom"; @@ -8,15 +8,15 @@ import { HTMLElementConstructor } from "./base-bridge-element"; import { PatchedElement, PatchedSlotElement, PatchedNode, PatchedIframeElement, PatchedCustomElement } from '../faux-shadow/faux'; // Using a WeakMap instead of a WeakSet because this one works in IE11 :( -const FromIteration: WeakMap = new WeakMap(); +const FromIteration: WeakMap = new WeakMap(); // dynamic children means it was generated by an iteration // in a template, and will require a more complex diffing algo. -export function markAsDynamicChildren(children: VNodes | VText) { +export function markAsDynamicChildren(children: VNodes) { FromIteration.set(children, 1); } -export function hasDynamicChildren(children: VNodes | VText): boolean { +export function hasDynamicChildren(children: VNodes): boolean { return FromIteration.has(children); } From 44cbf3f430bfb27d95772f36499a356173ec68d3 Mon Sep 17 00:00:00 2001 From: David Turissini Date: Wed, 7 Nov 2018 16:00:44 -0800 Subject: [PATCH 5/5] fix(engine): review feedback --- packages/lwc-engine/src/framework/api.ts | 5 ++++- .../duplicate-text-rendering.spec.js | 1 + 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/packages/lwc-engine/src/framework/api.ts b/packages/lwc-engine/src/framework/api.ts index 3c4aedc1e7..0457fcda08 100644 --- a/packages/lwc-engine/src/framework/api.ts +++ b/packages/lwc-engine/src/framework/api.ts @@ -459,7 +459,10 @@ export function f(items: any[]): any[] { const len = items.length; const flattened: VNodes = []; - // all flattened nodes should be marked as dynamic + // all flattened nodes should be marked as dynamic because + // flattened nodes are because of a conditional or iteration. + // We have to mark as dynamic because this could switch from an + // iterator to "static" text at any time. // TODO: compiler should give us some sort of indicator // to describe whether a vnode is dynamic or not markAsDynamicChildren(flattened); diff --git a/packages/lwc-integration/src/components/rendering/test-duplicate-text-rendering/duplicate-text-rendering.spec.js b/packages/lwc-integration/src/components/rendering/test-duplicate-text-rendering/duplicate-text-rendering.spec.js index c7dfbdb6d2..c4f6a525cc 100644 --- a/packages/lwc-integration/src/components/rendering/test-duplicate-text-rendering/duplicate-text-rendering.spec.js +++ b/packages/lwc-integration/src/components/rendering/test-duplicate-text-rendering/duplicate-text-rendering.spec.js @@ -8,6 +8,7 @@ describe('Dynamic text nodes rendering duplicate text', () => { it('should not render duplicate text', function () { browser.click('integration-duplicate-text-rendering'); + assert.notDeepEqual(browser.getText('integration-duplicate-text-rendering'), 'ab'); assert.deepEqual(browser.getText('integration-duplicate-text-rendering'), 'b'); }); });