test(integration): Add integration test for dom synchronization

[midzelis]

Details

Adds a testcase for https://playground.sfdc.es/projects/BJ0IY7ytM

Does this PR introduce a breaking change?

• Yes
• [x ] No

If yes, please describe the impact and migration path for existing applications:
Please check if your PR fulfills the following requirements:

[salesforce-best-lwc-internal]

Benchmark comparison

Base commit: 9fedf5a | Target commit: b1a1190

benchmark	base(9fedf5a)	target(b1a1190)	trend
table-append-1k.benchmark:benchmark-table/append/1k	307.98 (± 13.59 ms)	266.82 (± 2.14 ms)	👍
table-clear-1k.benchmark:benchmark-table/clear/1k	16.70 (± 1.08 ms)	14.71 (± 0.49 ms)	👍
table-create-10k.benchmark:benchmark-table/create/10k	1505.27 (± 20.41 ms)	1586.46 (± 35.51 ms)	👎
table-create-1k.benchmark:benchmark-table/create/1k	160.76 (± 1.60 ms)	170.10 (± 3.43 ms)	👎
table-update-10th-1k.benchmark:benchmark-table/update-10th/1k	137.40 (± 4.53 ms)	152.46 (± 3.99 ms)	👎
tablecmp-append-1k.benchmark:benchmark-table-component/append/1k	341.38 (± 4.45 ms)	357.58 (± 4.67 ms)	👎
tablecmp-clear-1k.benchmark:benchmark-table/clear/1k	33.93 (± 1.24 ms)	35.76 (± 1.99 ms)	👎
tablecmp-create-10k.benchmark:benchmark-table-component/create/10k	2603.70 (± 25.09 ms)	2706.32 (± 30.07 ms)	👎
tablecmp-create-1k.benchmark:benchmark-table-component/create/1k	295.26 (± 4.53 ms)	301.87 (± 7.06 ms)	👌
tablecmp-update-10th-1k.benchmark:benchmark-table-component/update-10th/1k	165.15 (± 6.72 ms)	143.71 (± 3.30 ms)	👍

[midzelis]

I've tracked down the change that originally fixed this test. It was fixed early in 0.17.16 development by 66454b4 committed by @caridy on Feb 7, 2018. Running this test on anything earlier will cause it to fail. There were a lot of changes in that commit, so unclear exactly which change actually fixed the test.

[caridy]

you are missing the key attribute here. Which is now a compiler error (in 0.18.x). Without that, events and props might exhibit the wrong value.

[midzelis]

The values are unique in this example, but I don't think this is the problem. I can make key={option.value}

[caridy]

an id of some sort must be part of the final array of items so it can be used in the template.

[caridy]

I'm not sure what exactly are you trying to prove here. You're manipulating internal state, and then pressing the internal button, and then looking at the value of the internal checkboxes to try to validate that the button updated the UI, but that's not how raptor works. Raptor works by hiding the internals, and exposing states that are only observable from outside, independently of what the user is seeing in the UI.

[pmdartus]

I spent half an hour debugging with @midzelis this issue this morning. Here are a couple of observations.

First attempt

<template>
    <input type="checkbox" checked={checkbox.isChecked} value="My checkbox">
    <button onclick={setUncheck}>Uncheck</button>
</template>

import { Element, track } from 'engine';

export default class LightningCheckboxGroup extends Element {
    @track checkbox = {
        isChecked: false
    };

    setUncheck() {
        this.checkbox = {
            isChecked: false
        };
    }
}

If you click on the checkbox and then click on the uncheck button, the checkbox doesn't get unchecked.

When clicking on the button, the template gets evaluated, and the diffing algo compares the old state of the VDom with the new state. But because the old VDom node checked prop is false and the new VDom node checked prop is false as well, the actual DOM node is not patched.

Second attempt

In order to get around this issue, we decided to add a change handler on the input tag and reflect the state checkbox state in a tracked property when it changes.

<template>
    <input type="checkbox" checked={checkbox.isChecked} onchange={handleChange} value="My checkbox">
    <button onclick={setUncheck}>Uncheck</button>
</template>

import { Element, track } from 'engine';

export default class LightningCheckboxGroup extends Element {
    @track checkbox = {
        isChecked: false
    };

    setUncheck() {
        this.checkbox = {
            isChecked: false
        };
    }

	handleChange(evt) {
        this.checkbox = {
            isChecked: evt.target.checked,
        };
    }
}

Everything works as expected. But now every time the checked property get toggled, the props update complains Unnecessary update of property "checked" in the element <input>..

[caridy]

jaja! @pmdartus you guys wasted 1h then! I knew exactly what's doing on here. you should have asked.

first attempt is incorrect, because you don't track the state of the checkbox, and the diffing algo doesn't know what to do. so, that's expected.

second attempt is correct, but we have a bug (for a long time) in raptor, that is only taking into account "value" as especial, instead of checking for all properties that have an initialization-only attribute, which are few of them. This line have to be updated: https://github.com/salesforce/lwc/blob/master/packages/lwc-engine/src/framework/modules/props.ts#L40

but it is tricky without introducing a regression. Last time we spoke about this, we were saying that maybe a hash table with those names, and a check for existent in that table can be faster than the string comparison for each of them in that if statement. feel free to send a PR.

[midzelis]

@caridy I'm not sure I agree that the first attempt is incorrect. I could 'track' the state of the checkbox by in a different private variable, i.e. 'this.deferredState' . Then, the button would simply assign the deferredState to isChecked(). The thing is, the diffing algorithm runs. Notices that old vdom state == current state, and doesn't need to update dom. However, I think it should also check the current state of the element, and bring that up to date with the vnode state.

[byao]

already fixed with #341