fix(engine): refactor boundary protection for render phase #1507

ravijayaramappa · 2019-09-13T01:23:50Z

Details

User land code is protected from making dangerous mutation during rendering phase.
Rendering a component is two steps, first the render() is invoked and then the returned template is evaluated.
The userland code was being protected using one global flag called isRendering.
In between these two steps, the engine is executing its own routine and does not need to be protected from dangerous mutations.

This PR separated the flag into two. One for render invocation and another for template evaluation.

Addresses issue #1506

Does this PR introduce breaking changes?

✅ No, it does not introduce breaking changes.

If yes, please describe the impact and migration path for existing applications.

The PR fulfills these requirements:

Have tests for the proposed changes been added? ✅
Have you followed these instructions to clearly describe the issue being fixed or feature enhanced? ✅

test case: A parent switches template and a child being disconnected sets a tracked property in disconnectedCallback

packages/@lwc/engine/src/framework/template.ts

-    }
+    const isEvaluatingTemplateInception = isEvaluatingTemplate;
+    let vnodes: VNodes = [];
+    runWithBoundaryProtection(


ravijayaramappa · 2019-09-13T01:28:37Z

packages/@lwc/engine/src/framework/invoker.ts

@@ -124,6 +124,7 @@ export function invokeComponentRenderMethod(vm: VM): VNodes {
        () => {
            // job
            const html = callHook(component, render);
+            isRendering = false;


@caridy Should we call this flag isRenderBeingInvoked or something like that.

yeah, feel free to rename it.

this line can't be done here... because if the render() method throws, the state is never restored! also inception is not preserved when you assign the false...

caridy · 2019-09-13T18:57:40Z

packages/@lwc/engine/src/framework/template.ts

+        () => {
+            // invoke the selected template.
+            vnodes = html.call(undefined, api, component, cmpSlots, context.tplCache!);
+            const { styleVNode } = context;


this block should be outside of the protection.

caridy · 2019-09-13T18:58:04Z

packages/@lwc/engine/src/framework/template.ts

+            }
+
+            if (process.env.NODE_ENV !== 'production') {
+                assert.invariant(


imagine that this throws an error... that will be captured by the boundary... that's not what we want.

caridy

still not entirely convinced that this is safe.

caridy · 2019-09-20T01:16:36Z

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

@@ -609,10 +615,10 @@ export function ll(
    id: string,
    context?: (...args: any[]) => any
 ): EventListener {
+    const vmBeingRendered: VM | null = getVMBeingRendered();


you should call this vm as it was before, because it is using in the next turn where vmBeingRendered is not longer set, it is confusing to call it like that.

caridy · 2019-09-20T01:56:06Z

packages/@lwc/engine/src/framework/invoker.ts

        }
    );
-    return result || [];
+    return isUndefined(html) ? [] : evaluateTemplate(vm, html);


we could probably make this more clear, maybe adding a comment saying that html can only be undefined if there is an error on render.call() that is captured by a boundary.

Note: This PR introduces a slight inconsistency in the way we handle invalid templates returned by invoking render().
if the render() returns an undefined

In dev mode, we assert and throw, invoke errorCallback in boundary protection

In prod mode, we do not throw. We return a blank template to gracefully degrade

if the render() returns a null/string/non-function

In dev mode, we assert and throw, invoke errorCallback in boundary protection

In prod mode, we do not assert, the framework will encounter a TypeError while trying to use the return value like a function. We invoke boundary protection.

I feel, throwing in dev mode and returning a blank template in prod mode is better user experience. Want to hear opinions.

this is not the same we have in master today... in master today, if you return undefined, in dev or prod, it throws.

in fact it is not about undefined, it is about a registered template.

caridy

minor things... but it is looking good.

salesforce-best-lwc-internal · 2019-09-20T20:23:55Z

Benchmark results

Base commit: 705b587 | Target commit: dc1cb3a

lwc-engine-benchmark

table-append-1k	metric	base(`705b587`)	target(`dc1cb3a`)	trend

table-clear-1k	metric	base(`705b587`)	target(`dc1cb3a`)	trend

table-create-10k	metric	base(`705b587`)	target(`dc1cb3a`)	trend

table-create-1k	metric	base(`705b587`)	target(`dc1cb3a`)	trend

table-update-10th-1k	metric	base(`705b587`)	target(`dc1cb3a`)	trend

tablecmp-append-1k	metric	base(`705b587`)	target(`dc1cb3a`)	trend

tablecmp-clear-1k	metric	base(`705b587`)	target(`dc1cb3a`)	trend

tablecmp-create-10k	metric	base(`705b587`)	target(`dc1cb3a`)	trend

tablecmp-create-1k	metric	base(`705b587`)	target(`dc1cb3a`)	trend

tablecmp-update-10th-1k	metric	base(`705b587`)	target(`dc1cb3a`)	trend

wc-append-1k	metric	base(`705b587`)	target(`dc1cb3a`)	trend

wc-clear-1k	metric	base(`705b587`)	target(`dc1cb3a`)	trend

wc-create-10k	metric	base(`705b587`)	target(`dc1cb3a`)	trend

wc-create-1k	metric	base(`705b587`)	target(`dc1cb3a`)	trend

wc-update-10th-1k	metric	base(`705b587`)	target(`dc1cb3a`)	trend

salesforce-best-lwc-internal · 2019-09-20T20:38:44Z

Benchmark results

Base commit: 705b587 | Target commit: dc1cb3a

lwc-engine-benchmark

table-append-1k	metric	base(`705b587`)	target(`dc1cb3a`)	trend

table-clear-1k	metric	base(`705b587`)	target(`dc1cb3a`)	trend

table-create-10k	metric	base(`705b587`)	target(`dc1cb3a`)	trend

table-create-1k	metric	base(`705b587`)	target(`dc1cb3a`)	trend

table-update-10th-1k	metric	base(`705b587`)	target(`dc1cb3a`)	trend

tablecmp-append-1k	metric	base(`705b587`)	target(`dc1cb3a`)	trend

tablecmp-clear-1k	metric	base(`705b587`)	target(`dc1cb3a`)	trend

tablecmp-create-10k	metric	base(`705b587`)	target(`dc1cb3a`)	trend

tablecmp-create-1k	metric	base(`705b587`)	target(`dc1cb3a`)	trend

tablecmp-update-10th-1k	metric	base(`705b587`)	target(`dc1cb3a`)	trend

wc-append-1k	metric	base(`705b587`)	target(`dc1cb3a`)	trend

wc-clear-1k	metric	base(`705b587`)	target(`dc1cb3a`)	trend

wc-create-10k	metric	base(`705b587`)	target(`dc1cb3a`)	trend

wc-create-1k	metric	base(`705b587`)	target(`dc1cb3a`)	trend

wc-update-10th-1k	metric	base(`705b587`)	target(`dc1cb3a`)	trend

caridy · 2019-09-25T02:26:33Z

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

@@ -609,10 +615,10 @@ export function ll(
    id: string,
    context?: (...args: any[]) => any
 ): EventListener {
-    if (isNull(vmBeingRendered)) {
+    const vm: VM | null = getVMBeingRendered();


unnecessary type declaration for const.

caridy

LGTM, probably adding more tests to cover the cases of the original behavior, so we don't break it in the future.

salesforce-best-lwc-internal · 2019-09-25T03:06:05Z

Benchmark results

Base commit: c6cde9c | Target commit: ea776f2

lwc-engine-benchmark

table-append-1k	metric	base(`c6cde9c`)	target(`ea776f2`)	trend

table-clear-1k	metric	base(`c6cde9c`)	target(`ea776f2`)	trend

table-create-10k	metric	base(`c6cde9c`)	target(`ea776f2`)	trend

table-create-1k	metric	base(`c6cde9c`)	target(`ea776f2`)	trend

table-update-10th-1k	metric	base(`c6cde9c`)	target(`ea776f2`)	trend

tablecmp-append-1k	metric	base(`c6cde9c`)	target(`ea776f2`)	trend

tablecmp-clear-1k	metric	base(`c6cde9c`)	target(`ea776f2`)	trend

tablecmp-create-10k	metric	base(`c6cde9c`)	target(`ea776f2`)	trend

tablecmp-create-1k	metric	base(`c6cde9c`)	target(`ea776f2`)	trend

tablecmp-update-10th-1k	metric	base(`c6cde9c`)	target(`ea776f2`)	trend

wc-append-1k	metric	base(`c6cde9c`)	target(`ea776f2`)	trend

wc-clear-1k	metric	base(`c6cde9c`)	target(`ea776f2`)	trend

wc-create-10k	metric	base(`c6cde9c`)	target(`ea776f2`)	trend

wc-create-1k	metric	base(`c6cde9c`)	target(`ea776f2`)	trend

wc-update-10th-1k	metric	base(`c6cde9c`)	target(`ea776f2`)	trend

ravijayaramappa · 2019-10-03T17:32:25Z

Will add tests for the original behavior when this change will be ported to master branch.

* test: automation for issue#1506 test case: A parent switches template and a child being disconnected sets a tracked property in disconnectedCallback * fix: mark template evaluation as a separate global flag * fix: split boundary protection for render into two steps

* fix(engine): refactor boundary protection for render phase (#1507) * test: automation for issue#1506 test case: A parent switches template and a child being disconnected sets a tracked property in disconnectedCallback * fix: mark template evaluation as a separate global flag * fix: split boundary protection for render into two steps * test: testcase for an existing behavior test: run test only if global promise rejection handler is supported

ravijayaramappa added 2 commits September 12, 2019 17:12

test: automation for issue#1506

1bd4618

test case: A parent switches template and a child being disconnected sets a tracked property in disconnectedCallback

fix: mark template evaluation as a separate global flag

f480707

ravijayaramappa requested review from caridy, ekashida and jodarove September 13, 2019 01:23

ravijayaramappa changed the title ~~fix(engine): separate global flag for render invocation and template evaluation~~ fix[engine]: separate global flag for render invocation and template evaluation Sep 13, 2019

ravijayaramappa changed the title ~~fix[engine]: separate global flag for render invocation and template evaluation~~ fix[engine]: refactor boundary protection for render phase Sep 13, 2019

ravijayaramappa changed the title ~~fix[engine]: refactor boundary protection for render phase~~ fix(engine): refactor boundary protection for render phase Sep 13, 2019

ravijayaramappa commented Sep 13, 2019

View reviewed changes

packages/@lwc/engine/src/framework/template.ts

}

const isEvaluatingTemplateInception = isEvaluatingTemplate;

let vnodes: VNodes = [];

runWithBoundaryProtection(

This comment was marked as outdated.

Sign in to view

This comment was marked as resolved.

Sign in to view

ravijayaramappa commented Sep 13, 2019

View reviewed changes

salesforce deleted a comment from salesforce-best-lwc-internal bot Sep 13, 2019

caridy reviewed Sep 13, 2019

View reviewed changes

fix: move engine logic out of boundary protection

7ef4ad3

salesforce deleted a comment from salesforce-best-lwc-internal bot Sep 13, 2019

caridy suggested changes Sep 14, 2019

View reviewed changes

fix: split boundary protection for render into two steps

a05f4da

caridy reviewed Sep 20, 2019

View reviewed changes

caridy suggested changes Sep 20, 2019

View reviewed changes

salesforce deleted a comment from salesforce-best-lwc-internal bot Sep 20, 2019

fix: pr feedback and test updates

dc1cb3a

ravijayaramappa added 2 commits September 24, 2019 18:07

fix: restore error handling to original

bdb1909

test: restore test to original

ea776f2

This comment has been minimized.

Sign in to view

caridy reviewed Sep 25, 2019

View reviewed changes

caridy approved these changes Sep 25, 2019

View reviewed changes

ravijayaramappa merged commit 926f8f0 into winter20 Oct 3, 2019

ravijayaramappa deleted the ravi/winter20/rendering-invariant-refactor/W-6572707 branch October 3, 2019 17:31

ravijayaramappa mentioned this pull request Oct 3, 2019

fix(engine): refactor boundary protection for render phase #1551

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(engine): refactor boundary protection for render phase #1507

fix(engine): refactor boundary protection for render phase #1507

ravijayaramappa commented Sep 13, 2019 •

edited

Loading

This comment was marked as outdated.

This comment was marked as resolved.

ravijayaramappa Sep 13, 2019

caridy Sep 14, 2019

caridy Sep 14, 2019

caridy Sep 13, 2019

caridy Sep 13, 2019

caridy left a comment

caridy Sep 20, 2019

caridy Sep 20, 2019

ravijayaramappa Sep 20, 2019 •

edited

Loading

caridy Sep 20, 2019

caridy Sep 20, 2019

caridy left a comment

salesforce-best-lwc-internal bot commented Sep 20, 2019

salesforce-best-lwc-internal bot commented Sep 20, 2019

This comment has been minimized.

caridy Sep 25, 2019

caridy left a comment

salesforce-best-lwc-internal bot commented Sep 25, 2019

ravijayaramappa commented Oct 3, 2019

fix(engine): refactor boundary protection for render phase #1507

fix(engine): refactor boundary protection for render phase #1507

Conversation

ravijayaramappa commented Sep 13, 2019 • edited Loading

Details

Does this PR introduce breaking changes?

The PR fulfills these requirements:

This comment was marked as outdated.

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caridy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ravijayaramappa Sep 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caridy left a comment

Choose a reason for hiding this comment

salesforce-best-lwc-internal bot commented Sep 20, 2019

Benchmark results

lwc-engine-benchmark

salesforce-best-lwc-internal bot commented Sep 20, 2019

Benchmark results

lwc-engine-benchmark

This comment has been minimized.

Choose a reason for hiding this comment

caridy left a comment

Choose a reason for hiding this comment

salesforce-best-lwc-internal bot commented Sep 25, 2019

Benchmark results

lwc-engine-benchmark

ravijayaramappa commented Oct 3, 2019

ravijayaramappa commented Sep 13, 2019 •

edited

Loading

ravijayaramappa Sep 20, 2019 •

edited

Loading