refactor(engine): add support for template hot swapping #1860

caridy · 2020-05-05T18:01:09Z

Details

Experimental template, component and stylesheet hot-swapping for @diervo's crazy idea!
Fix #1869

Does this PR introduce breaking changes?

✅ No, it does not introduce breaking changes.

Example of usage

// In this example we use websockets to notify the client that a particular module has changed. We refetch the new modules and ask the engine to hot swap them.

import { swapTemplate, swapStyle, swapComponent } from 'lwc';
export function initHMR(): void {
    const socket = new WebSocket(`ws://${location.host}`);

    socket.addEventListener('message', async ({ data }) => {
        const event = JSON.parse(data);

        if (event.eventType === 'moduleUpdate') {
            const {
                oldUri,
                newUri,
                module: { namespace, name },
            } = event.payload;

            const oldModule = await import(oldUri);
            const newModule = await import(newUri);
            if (name.endsWith('html')) {
                console.log(`Swapping html template for module "${namespace}/${name}"`);
                swapTemplate(oldModule.default, newModule.default);
            } else if (name.endsWith('css')) {
                swapStyle(oldModule.default[0], newModule.default[0]);
            } else {
                swapComponent(oldModule.default, newModule.default);
            }
        } else {
            console.log('ws: ', event);
        }
    });
}

caridy · 2020-05-05T18:01:53Z

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

+        // in dev-mode, we support hot swapping of templates, which means that
+        // the component instance might be attempting to use an old version of
+        // the template, while internally, we have a replacement for it.
+        html = getTemplateOrSwappedTemplate(html);


this returns the same template provided, or the new version of it based on calls to registerTemplateSwap

caridy · 2020-05-05T18:02:42Z

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

                        // that is not provided by the component instance.
                        validateFields(vm, html);
+                        // add the VM to the list of host VMs that can be re-rendered if html is swapped
+                        addHotVM(vm);


after the vm and the new template were associated, the VM is added to the a set of hot VMs linked to the template, in case the template gets swapped later on.

caridy · 2020-05-05T18:03:30Z

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

 // old vnode.children is removed from the DOM.
 export function removeVM(vm: VM) {
    if (process.env.NODE_ENV !== 'production') {
+        removeHotVM(vm);


this is to avoid leaking VM instances for components that were removed, but their template still available for new instances. this just complements addHotVM

caridy · 2020-05-05T18:04:26Z

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


 export { setFeatureFlag, setFeatureFlagForTest } from '@lwc/features';
+
+export { registerTemplateSwap } from './hot-swaps';


@diervo we need to restrict this in platform, and this is the one we call with the old template and the new template functions from outside.

…ad of throwing

packages/integration-karma/test/swapping/templates/index.spec.js

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

packages/@lwc/engine/src/framework/hot-swaps.ts

pmdartus · 2020-05-14T14:06:45Z

packages/integration-karma/test/swapping/templates/index.spec.js

@@ -0,0 +1,36 @@
+import { createElement } from 'lwc';


Let's also add tests:

for components

for stylesheets swapping for both synthetic shadow and native shadow.

in the making

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

pmdartus · 2020-05-14T14:15:51Z

packages/@lwc/engine/src/framework/hot-swaps.ts

+            // created from a template, root elements can't be swapped because we
+            // don't have a way to force the creation of the element with the same state
+            // of the current element.
+            if (!isNull(owner) && isFalse(owner.isDirty)) {


Vue 3 automatically reloads the page if the root element is modified: https://github.com/vuejs/vue-next/blob/master/packages/runtime-core/src/hmr.ts#L108-L118.
I think it makes sense to do that automatically or to add a flag reloadIfNeeded or something like that to registerComponentSwap to reload the page if the component can't be swapped.

@caridy said root might be tricky, I think hot swapping the root doesn't look like a terrible corner case to have.

@diervo what @pmdartus is saying is to reload the entire page (which we can definitely do, if the VM doesn't have an owner, which means it is a root element. I can do that for sure.

Another approach would be to notify the caller that the swap can't be done and it's to the caller to decide what to do: display a banner on the page with a reload button, log a warning or an error, ...

this is more complicated, because it is not a boolean operation:
a) the swap was possible (on at least 1 instance)
b) the swap was not possible in X active instances
c) the swap is not needed (no associated instance)

We could make it true/false if we say:

true is problem was not encounter, which is a and c

false if at least one instance was root (which mean we were not able to swap it)

ok, figured a way for this that works fine... the apis are going to return a boolean, whether or not the operation succeed or not..., in the case of the template and style they will (for now) always return true. And for components, it will return false if a root instance is changing, so @diervo code can refresh the entire page

pmdartus · 2020-05-14T16:00:34Z

@diervo Is it possible to add a small snippet in this PR explaining how this API is planned to be used with other tools. I don't think we need to fully document it, but having a small snippet explaining how this is intended to work would be great.

diervo · 2020-05-14T17:33:04Z

@pmdartus done

pmdartus · 2020-05-15T09:02:28Z

I have been looking at how Webpack and parcel are supporting HMR, and I don't have found yet a way to keep a reference to the old module, which makes it impossible to invoke swapTemplate for example with the old template. It appears that other frameworks like Vue are associating swappable modules with a unique identifier to avoid keeping the old reference around.

Let's make sure that those new APIs are compatible with Webpack to leverage them in lwc-create-app.

/cc @muenzpraeger

diervo · 2020-05-15T16:48:11Z

@pmdartus can't we just hook in the process so we are the ones keeping track? That what I do in LWR anyway. But moreover the current implementation seems to be working passing always the oldest reference FYI @caridy

Co-authored-by: Pierre-Marie Dartus <p.dartus@salesforce.com>

caridy · 2020-05-20T21:03:02Z

@pmdartus @diervo I have updated the PR with all the suggested fixes... only component and style tests are missing, working on them... can you do another pass?

salesforce-best-lwc-internal · 2020-05-20T21:16:58Z

🥳 Performance Improvement

Best has detected that there is a 5.0% performance improvement across your benchmarks.

Please click here to see more details.

Click to view significantly changed benchmarks

lwc-engine-benchmark

✅ Improvements	base (`76812fb`)	target (`c06d867`)	trend
table-append-1k.benchmark/benchmark-table/append/1k	334.14 (± 1.98ms)	269.63 (± 2.26ms)	-64.5ms (19.3%)
table-clear-1k.benchmark/benchmark-table/clear/1k	17.99 (± 0.20ms)	16.44 (± 0.22ms)	-1.6ms (8.6%)
table-create-10k.benchmark/benchmark-table/create/10k	1798.19 (± 18.20ms)	1442.16 (± 14.12ms)	-356.0ms (19.8%)
table-create-1k.benchmark/benchmark-table/create/1k	216.75 (± 1.50ms)	176.79 (± 1.03ms)	-40.0ms (18.4%)
table-update-10th-1k.benchmark/benchmark-table/update-10th/1k	172.84 (± 2.50ms)	145.44 (± 1.78ms)	-27.4ms (15.9%)
tablecmp-append-1k.benchmark/benchmark-table-component/append/1k	432.89 (± 2.47ms)	365.06 (± 2.46ms)	-67.8ms (15.7%)
tablecmp-clear-1k.benchmark/benchmark-table-component/clear/1k	10.29 (± 0.15ms)	9.21 (± 0.16ms)	-1.1ms (10.5%)

packages/@lwc/engine/src/framework/hot-swaps.ts

ravijayaramappa · 2020-05-21T00:03:32Z

packages/@lwc/engine/src/framework/hot-swaps.ts

+import { Template } from './template';
+import { StylesheetFactory } from './stylesheet';
+
+const swappedTemplateMap = new WeakMap<Template, Template>();


Going with Diego's concept of "everything is a module", we could potentially use 1 WeakMap of modules instead of 3 WeakMaps for the different modules types.

right, but because the reaction mechanism depends on the type that you're swapping, we have 3 of them.

packages/@lwc/engine/src/framework/hot-swaps.ts

…into caridy/template-hot-swap

salesforce-best-lwc-internal · 2020-05-21T02:59:06Z

🥳 Performance Improvement

Best has detected that there is a 6.6% performance improvement across your benchmarks.

Please click here to see more details.

Click to view significantly changed benchmarks

lwc-engine-benchmark

✅ Improvements	base (`76812fb`)	target (`c447066`)	trend
table-append-1k.benchmark/benchmark-table/append/1k	334.14 (± 1.98ms)	266.29 (± 1.75ms)	-67.9ms (20.3%)
table-clear-1k.benchmark/benchmark-table/clear/1k	17.99 (± 0.20ms)	15.85 (± 0.26ms)	-2.1ms (11.9%)
table-create-10k.benchmark/benchmark-table/create/10k	1798.19 (± 18.20ms)	1421.86 (± 14.41ms)	-376.3ms (20.9%)
table-create-1k.benchmark/benchmark-table/create/1k	216.75 (± 1.50ms)	173.94 (± 1.54ms)	-42.8ms (19.7%)
table-update-10th-1k.benchmark/benchmark-table/update-10th/1k	172.84 (± 2.50ms)	141.27 (± 1.67ms)	-31.6ms (18.3%)
tablecmp-append-1k.benchmark/benchmark-table-component/append/1k	432.89 (± 2.47ms)	362.53 (± 2.73ms)	-70.4ms (16.3%)
tablecmp-clear-1k.benchmark/benchmark-table-component/clear/1k	10.29 (± 0.15ms)	8.86 (± 0.19ms)	-1.4ms (13.9%)

pmdartus · 2020-05-22T05:50:49Z

packages/@lwc/engine/src/framework/hot-swaps.ts

+        // the styles will be reset, along with the content of the
+        // shadow, this way we can guarantee that the styles will be
+        // recalculated, and applied.
+        vm.cmpTemplate = () => [];


We are leaking internal details about how the engine decides to re-render or not a component here (eg. resetting the cmpTemplate). What do you think about exposing a method out of the VM.ts like forceHydrate to scope this?

I'm fine with that too... will do

caridy · 2020-05-22T17:48:16Z

packages/@lwc/engine/src/framework/hot-swaps.ts

+    }
+}
+
+export function swapTemplate(oldTpl: Template, newTpl: Template): boolean {


@diervo @pmdartus @ravijayaramappa do you think we should check the arguments to be a valid register template?

IMO, no. It will be checked before evaluation in evaluateTemplate.

!isTemplateRegistered(html)

That check is lazy, the template is validated before it is used. So if you register a template and the engine never uses it, the check is avoided.

caridy · 2020-05-22T17:48:32Z

packages/@lwc/engine/src/framework/hot-swaps.ts

+): boolean {
+    if (process.env.NODE_ENV !== 'production') {
+        swappedComponentMap.set(oldComponent, newComponent);
+        return rehydrateHotComponent(oldComponent);


@diervo @pmdartus @ravijayaramappa do you think we should check the arguments to be a valid registered constructor?

Same here, getComponentInternalDef() takes care of the validation.

@ravijayaramappa the problem here is that this will happen in the next tick, not when you call for the swap

caridy · 2020-05-22T17:51:29Z

packages/@lwc/engine/src/framework/hot-swaps.ts

+
+export function swapStyle(oldStyle: StylesheetFactory, newStyle: StylesheetFactory): boolean {
+    if (process.env.NODE_ENV !== 'production') {
+        swappedStyleMap.set(oldStyle, newStyle);


I don't think we have a way to validate whether a style is a compiled style or not... probably we should eventually do some registration from compiler to make sure that people don't mess around with this.

caridy · 2020-05-22T18:00:42Z

packages/@lwc/engine/src/framework/hot-swaps.ts

+    const list = activeComponents.get(Ctor);
+    let canRefreshAllInstances = true;
+    list?.forEach((vm) => {
+        const { owner } = vm;


another point to clear up about this PR is whether or not we should clean up the active VMs associated to the provided Ctor. IMO, this is not a problem... yes, you have things in the weakmap that are anchoring those VMs to old versions of Ctor, and probably a new one too after the rehydration... but that should not represent a problem at all... this is a dev mode feature anyway. But if you feel that we should do the clean up, I can do that, it is simpler.

I do think we should cleanup when possible. The 6 weakmaps makes me nervous about memory leaks.
The three weak maps for the swaps allow circular references. The other three weakmaps create a link between the swaps and the vms. Now, the VMs themselves have a strong reference to the template and the component constructors.

caridy · 2020-05-28T04:36:03Z

@ravijayaramappa @diervo this is ready for a final pass!

caridy · 2020-05-28T04:37:22Z

packages/@lwc/engine/src/framework/hot-swaps.ts

+            // in the vm, we only hold the selected (already swapped template)
+            // but the styles attached to the template might not be the actual
+            // active ones, but the swapped versions of those.
+            stylesheet = getStyleOrSwappedStyle(stylesheet);


@ravijayaramappa this is not very elegant. I think we should start storing the styles into the vm, after all the swapping and computations. then here we don't need to do this.

caridy · 2020-05-28T04:39:15Z

packages/integration-karma/test/swapping/styles/index.spec.js

+        const elm = createElement('x-simple', { is: Simple });
+        document.body.appendChild(elm);
+        expect(getComputedStyle(elm.shadowRoot.firstChild).display).toBe('block');
+        swapStyle(blockStyle[0], inlineStyle[0]);


@diervo what I noticed is that when you import a css file, what you get is the array, not the factory function. That's why I have to use [0] everywhere here... not ideal. The question is, how does your other part work? is it an array, or each individual one? we can certainly do the thing on the array identity from the import instead of individual style factories.

diervo · 2020-10-30T15:08:07Z

Closing in favor of #2071

refactor(engine): add support for templat hot swapping

4ab87b1

caridy requested a review from diervo May 5, 2020 18:01

caridy commented May 5, 2020

View reviewed changes

diervo changed the title ~~refactor(engine): add support for templat hot swapping~~ refactor(engine): add support for template hot swapping May 5, 2020

caridy and others added 6 commits May 6, 2020 00:09

refactor(engine): add support for style hot swapping

4a5bee4

refactor(engine): add support for component hot swapping

7cc4ca6

refactor(engine): making public API to do nothing in production inste…

c376312

…ad of throwing

refactor(engine): adding tests

87311e6

fix(engine): incorrect usage of shared list of vms

77a29b4

fix: component swap ctor check

7007eb9

pmdartus reviewed May 14, 2020

View reviewed changes

caridy and others added 3 commits May 20, 2020 16:27

Apply suggestions from code review

82c187f

Co-authored-by: Pierre-Marie Dartus <p.dartus@salesforce.com>

refactor(engine): review feedback

fdb7638

Merge branch 'master' into caridy/template-hot-swap

c06d867

ravijayaramappa reviewed May 20, 2020

View reviewed changes

packages/@lwc/engine/src/framework/hot-swaps.ts Show resolved Hide resolved

ravijayaramappa reviewed May 20, 2020

View reviewed changes

packages/@lwc/engine/src/framework/hot-swaps.ts Outdated Show resolved Hide resolved

ravijayaramappa reviewed May 20, 2020

View reviewed changes

packages/@lwc/engine/src/framework/hot-swaps.ts Outdated Show resolved Hide resolved

ravijayaramappa reviewed May 20, 2020

View reviewed changes

packages/@lwc/engine/src/framework/hot-swaps.ts Outdated Show resolved Hide resolved

ravijayaramappa reviewed May 21, 2020

View reviewed changes

packages/@lwc/engine/src/framework/hot-swaps.ts Outdated Show resolved Hide resolved

refactor(engine): feedback from ravi

309ed28

Merge branch 'caridy/template-hot-swap' of github.com:salesforce/lwc …

c447066

…into caridy/template-hot-swap

test(engine): adding swapComponent tests

a549457

pmdartus reviewed May 22, 2020

View reviewed changes

caridy commented May 22, 2020

View reviewed changes

ravijayaramappa mentioned this pull request May 22, 2020

[HMR] CSS: Provide a mechanism to register stylesheets #1887

Open

fix(engine): adding style swap tests and pr feedbacks

2d5b68d

caridy commented May 28, 2020

View reviewed changes

ravijayaramappa added this to the 230 milestone Jun 3, 2020

ravijayaramappa added the nomerge label Jun 3, 2020

diervo closed this Oct 30, 2020

diervo deleted the caridy/template-hot-swap branch November 25, 2020 22:09


		export { setFeatureFlag, setFeatureFlagForTest } from '@lwc/features';

		export { registerTemplateSwap } from './hot-swaps';

refactor(engine): add support for template hot swapping #1860

refactor(engine): add support for template hot swapping #1860

Uh oh!

Conversation

caridy commented May 5, 2020 • edited by ravijayaramappa Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Does this PR introduce breaking changes?

Example of usage

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

diervo May 14, 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!

Choose a reason for hiding this comment

Uh oh!

pmdartus commented May 14, 2020

Uh oh!

diervo commented May 14, 2020

Uh oh!

pmdartus commented May 15, 2020

Uh oh!

diervo commented May 15, 2020

Uh oh!

caridy commented May 20, 2020

Uh oh!

salesforce-best-lwc-internal bot commented May 20, 2020

🥳 Performance Improvement

lwc-engine-benchmark

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ravijayaramappa May 21, 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!

Uh oh!

salesforce-best-lwc-internal bot commented May 21, 2020

🥳 Performance Improvement

lwc-engine-benchmark

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

caridy May 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

caridy commented May 5, 2020 •

edited by ravijayaramappa

Loading

diervo May 14, 2020 •

edited

Loading

ravijayaramappa May 21, 2020 •

edited

Loading

caridy May 22, 2020 •

edited

Loading

ravijayaramappa May 22, 2020 •

edited

Loading