fix(engine): add keys to text vnodes #1943

jodarove · 2020-06-23T20:54:20Z

Details

When vnodes are allocated in slots, the process rewrites the vnode key, in the case of text vnodes (they don't have key), it will make 2 different text vnodes in the same slot, to have the same key.

Later on in this section diffing algo, createKeyToOldIdx will map incorrectly those 2 nodes, causing the algo to patch the incorrect vnode, which eventually triggers an error.

This PR fixes the issue by generating a key for text nodes. It also fixes #1637, improving createKeyToOldIdx since now all vnodes have key.

Note: For the review, the functionality is in the first commit (de8d086). The rest of the pr is updating the test snapshots.

Does this PR introduce breaking changes?

✅ No, it does not introduce breaking changes.

GUS work item

W-7292159

ravijayaramappa · 2020-06-23T21:27:40Z

packages/@lwc/engine-core/src/3rdparty/snabbdom/types.ts

@@ -63,7 +63,7 @@ export interface VText extends VNode {
    children: undefined;
    elm: Node | undefined;
    text: string;
-    key: undefined;
+    key: Key | undefined;


Suggested change

key: Key | undefined;

key: Key;

Also, can we now now update VNode interface to key: Key; and not override it in VElement and VText

ravijayaramappa · 2020-06-23T21:34:28Z

packages/@lwc/engine-core/src/framework/api.ts

@@ -77,8 +77,8 @@ export interface RenderAPI {
    ): VNode;
    i(items: any[], factory: () => VNode | VNode): VNodes;
    f(items: any[]): any[];
-    t(text: string): VText;
-    d(value: any): VNode | null;
+    t(text: string, data: ElementCompilerData): VText;


Should this be called NodeCompilerData? It would be non-intuitive to generate ElementCompilerData for a text node.
Although, ElementCompilerData and its parent VNodeData seems bloated for a text node.

Suggested change

t(text: string, data: ElementCompilerData): VText;

t(text: string, data: NodeCompilerData): VText;

in theory it could be just "key", i made it an object so it's compatible with the existing API (and if extended in the future the api does not need to change).

imo, adding key to VNodeData makes sense. what do you think?

ravijayaramappa · 2020-06-23T21:43:58Z

packages/@lwc/engine-core/src/framework/api.ts

-    const data = EmptyObject;
-    let sel, children, key, elm;
+export function t(text: string, data: CustomElementCompilerData): VText {
+    const { key } = data;


Is the data a newly created param from the compiler OR is it pre-existing param that was not used? Will this break any existing pre-compiled component definitions?

mm, good point, data is a new param from the compiler. i left the VText to be Key | undefined, trying to support the existing compilations, but i missed this case.

Though, we should probably dismiss existing pre-compiled components. thoughts?

Yes, all the precompiled components have to be dismissed.

ravijayaramappa · 2020-06-23T21:50:11Z

packages/@lwc/engine-core/src/framework/api.ts

-export function t(text: string): VText {
-    const data = EmptyObject;
-    let sel, children, key, elm;
+export function t(text: string, data: CustomElementCompilerData): VText {


Suggested change

export function t(text: string, data: CustomElementCompilerData): VText {

export function t(text: string, data: ElementCompilerData): VText {

ravijayaramappa · 2020-06-23T21:50:20Z

packages/@lwc/engine-core/src/framework/api.ts

@@ -519,11 +519,11 @@ export function t(text: string): VText {
 }

 // [d]ynamic value to produce a text vnode
-export function d(value: any): VNode | null {
+export function d(value: any, data: CustomElementCompilerData): VNode | null {


Suggested change

export function d(value: any, data: CustomElementCompilerData): VNode | null {

export function d(value: any, data: ElementCompilerData): VNode | null {

ekashida

I'm curious whether there would be a performance benefit to use the actual string of the text node as the key?

pmdartus

We should avoid creating short life objects when possible especially on the critical path.

pmdartus · 2020-06-24T07:58:43Z

packages/@lwc/engine-core/src/framework/api.ts

@@ -77,8 +77,8 @@ export interface RenderAPI {
    ): VNode;
    i(items: any[], factory: () => VNode | VNode): VNodes;
    f(items: any[]): any[];
-    t(text: string): VText;
-    d(value: any): VNode | null;
+    t(text: string, data: ElementCompilerData): VText;


Instead of creating a new object here, I would recommend passing directly the key a parameter to avoid creating a short life object.

Suggested change

t(text: string, data: ElementCompilerData): VText;

t(text: string, key: number): VText;

pmdartus · 2020-06-24T07:59:03Z

packages/@lwc/engine-core/src/framework/api.ts

-    t(text: string): VText;
-    d(value: any): VNode | null;
+    t(text: string, data: ElementCompilerData): VText;
+    d(value: any, data: ElementCompilerData): VNode | null;


Ditto. Also in this case, the returned value is a VText not a Vnode.

Suggested change

d(value: any, data: ElementCompilerData): VNode | null;

d(value: any, key: number): VText | null;

pmdartus · 2020-06-24T08:02:15Z

packages/@lwc/engine-core/src/framework/api.ts

-    const data = EmptyObject;
-    let sel, children, key, elm;
+export function t(text: string, data: CustomElementCompilerData): VText {
+    const { key } = data;


Yes, all the precompiled components have to be dismissed.

pmdartus · 2020-06-24T08:03:14Z

packages/integration-karma/test/rendering/slotted-text-diffing/index.spec.js

+                expect(text).toContain('false');
+
+                elm.toggleVisibility();
+                return Promise.resolve();


Not needed, the .then is invoked in the microtask after the render.

Suggested change

return Promise.resolve();

pmdartus · 2020-06-24T08:04:06Z

packages/integration-karma/test/rendering/slotted-text-diffing/index.spec.js

+        return Promise.resolve()
+            .then(() => {
+                const text = elm.shadowRoot.textContent;
+                expect(text).toContain('false');


We should match against the entire text content here to make sure that all the text is in the right place, not only the partial content.

pmdartus · 2020-06-24T08:04:50Z

packages/integration-karma/test/rendering/slotted-text-diffing/index.spec.js

+            })
+            .then(() => {
+                const text = elm.shadowRoot.textContent;
+                expect(text).toContain('second text');


pmdartus · 2020-06-24T08:07:21Z

packages/integration-karma/test/rendering/slotted-text-diffing/index.spec.js

+    it('should not confuse text vnodes when moving elements', function () {
+        const elm = createElement('x-container', { is: Container });
+        document.body.appendChild(elm);
+


Let's also assert the text content here before the first toogle.

pmdartus · 2020-06-24T08:09:20Z

@jodarove Also before moving forward, Best caught a ~8% regression on 2 benchmarks. We should investigate this moving forward with this PR.

jodarove · 2020-06-25T00:32:34Z

mmm, true about the performance, ill need to take a look. Alternatively, i opened #1946 which does not have the performance, and also fixes the issue. cc/ @pmdartus @ravijayaramappa @ekashida

jodarove · 2020-06-25T16:45:08Z

closed in favor of #1946

jodarove requested review from pmdartus, ekashida and ravijayaramappa June 23, 2020 21:16

ravijayaramappa requested changes Jun 23, 2020

View reviewed changes

ekashida reviewed Jun 24, 2020

View reviewed changes

pmdartus requested changes Jun 24, 2020

View reviewed changes

pmdartus reviewed Jun 24, 2020

View reviewed changes

jodarove force-pushed the jodarove/fix-diffing-algo-with-text branch 2 times, most recently from e6c6aaa to 623b42f Compare June 24, 2020 18:59

jodarove added 3 commits June 24, 2020 12:05

fix(engine): add keys to text vnodes

cc68984

fix: snapshot tests

faa1538

fix: rebase conflicts

43611c1

jodarove force-pushed the jodarove/fix-diffing-algo-with-text branch from 623b42f to 43611c1 Compare June 24, 2020 19:10

jodarove mentioned this pull request Jun 25, 2020

fix(synthetic-shadow): dont add key for slotted text vnode #1946

Merged

jodarove closed this Jun 25, 2020

jodarove deleted the jodarove/fix-diffing-algo-with-text branch June 25, 2020 16:45

	t(text: string, data: ElementCompilerData): VText;
	t(text: string, data: NodeCompilerData): VText;

	export function t(text: string, data: CustomElementCompilerData): VText {
	export function t(text: string, data: ElementCompilerData): VText {

	export function d(value: any, data: CustomElementCompilerData): VNode \| null {
	export function d(value: any, data: ElementCompilerData): VNode \| null {

	t(text: string, data: ElementCompilerData): VText;
	t(text: string, key: number): VText;

	d(value: any, data: ElementCompilerData): VNode \| null;
	d(value: any, key: number): VText \| null;

fix(engine): add keys to text vnodes #1943

fix(engine): add keys to text vnodes #1943

Uh oh!

Conversation

jodarove commented Jun 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Does this PR introduce breaking changes?

GUS work item

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jodarove Jun 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jodarove Jun 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ekashida left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pmdartus left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pmdartus commented Jun 24, 2020

Uh oh!

jodarove commented Jun 25, 2020

Uh oh!

jodarove commented Jun 25, 2020

Uh oh!

Uh oh!

jodarove commented Jun 23, 2020 •

edited

Loading

jodarove Jun 23, 2020 •

edited

Loading

jodarove Jun 23, 2020 •

edited

Loading

ekashida left a comment •

edited

Loading