refactor(engine-core): encapsulate renderer as an object and allow it to be injectable in vnodes

[caridy]

Details

Does this pull request introduce a breaking change?

• ✅ No, it does not introduce a breaking change. It just readjust the internals of the framework to accommodate the new mechanism of passing the renderer API around.

Does this pull request introduce an observable change?

• ⚠️ Yes, it does include an observable change.

It introduces a new API, the renderer object is now exported, even though is an exotic namespace object, it contains a lot of functions that are now public somehow, but kinda private in the sense that no one is supposed to use this directly, but only the compiled code.

• add a linting rule to avoid people arbitrarily importing renderer from lwc.

TODO

• compiler has to be updated to match the new api.ts shape, which is basically adding import { renderer } from 'lwc'; in the output modele, and passing renderer to some of the functions exported from api as well.

Potential Issues

The big problem with this approach is "performance", specifically, if LWS needs to tap into these functions, it will slow down everyone when in reality LWS is only interested on the 0.01% of the DOM APIs used by LWC templates. This poses a real problem.

[caridy]

there are now extracted from the renderer associated to the vm for a lightning element.

[caridy]

@nolanlawson I believe this abstraction can be removed completely now... seems unnecessary since no more optimization is possible for those import statements.

[caridy]

this is the counter part of the previous note @nolanlawson, also can be removed.

[caridy]

removing all these public apis... with this change, core package is useless by itself because it doesn't export the renderer methods needed by a compiled template, basically the compiled code must import or be linked to a production of a package that does have an export called renderer.

[caridy]

this file is to be renamed to renderer.ts

[caridy]

not sure if we want to just export an object, or to deal with exotic namespace objects via * as syntax.

[caridy]

import * is not good a determining types, probably another point to make the renderer an actual export of an object called renderer.

[pmdartus]

Let's make sure to address this before merging.

[caridy]

this is just the API that was previously defined in renderer.ts shape, but since it needs to be passed around, I Don't know if there is a way to define a type based on the shape of a module that can be imported. In any case, I did it manually.

[nolanlawson]

I mentioned this during Dev Sync, but I would prefer for us to take a slightly different approach. In particular, I would prefer not to have a renderer object that we call at runtime (renderer.getAttribute(), renderer.setCSSStyleProperty(), etc.) and pass around on the VM. Instead, I would prefer to have top-level functions imported from a module (import { getAttribute, setCSSStyleProperty } from 'somewhere').

In a previous PR, I demonstrated runtime performance gains by using top-level functions (#2598). We also had an unintentional runtime regression that would have been avoided if we had been using top-level functions at the time (#2562). I wasn't able to run the perf tests on this PR (there are functional errors), but I would expect to see a slight perf degradation from passing around the renderer object and calling methods on it.

Also, this PR increases the size of engine-dom.min.js from 49.5kB to 52.1kB (2.6kB increase). So there is a bundle size regression as well.

It seems that the goal is to be able to add hooks into the renderer so that Locker can run custom logic. But even in that case, there is only one renderer – it is a singleton. So I would prefer to make this hook a compile-time concern rather than a runtime concern.

One way to do this is the technique I originally used in #2598, which is to have a separate @lwc/renderer-abstract package that is imported by @lwc/engine-core. The expectation is that whoever is building their own version of LWC (e.g. @lwc/engine-dom, @lwc/engine-server, or @lwc/engine-dom-with-locker-hooks) would be responsible for supplying the implementation of this package and including it in the Rollup config.

Another solution is something like #2758 (comment), which is to hoist all the renderer functions into the lwc package. Then you would do e.g. import { getAttribute } from 'lwc'.

Both of these approaches add code complexity, but the benefit is that end-users do not pay the perf tax for extra runtime code to expose hooks (or to pass the renderer object around). I think this is especially important for use cases where consumers want the smallest possible bundle size and do not need Locker (e.g. off-core B2C), so we should be sensitive to the perf concerns of those use cases.

[caridy]

Instead, I would prefer to have top-level functions imported from a module (import { getAttribute, setCSSStyleProperty } from 'somewhere').

The problem is that the whole point of this PR is to create an indirection between the renderer, and the consumers of the renderer via the compiled template. Meaning, the template file itself doesn't know what methods of the renderer will be used by core, it only knows those that are used by template optimizations directly.

In terms of hooks, the idea here is that even core is subject to the transformation of the imported, in this case, the importer is the compiled template, and that's where locker can intervene. The two solutions that you mentioned can't really allow such thing.

[nolanlawson]

@caridy I don't follow you. In the model I propose, anyone who is building their own version of LWC (e.g. lwc-platform) would be able to substitute whatever renderer implementation they want. I'm proposing the exact same thing as you – I'm just moving the substitution into the Rollup compilation phase, rather than at runtime.

In this commit (2d548bd), I had a PoC of how this would work. The compiled template would do:

import { getAttribute } from '@lwc/renderer-abstract'

And then we'd use a simple Rollup plugin to substitute at build time:

   plugins: [
        {
            resolveId(importee) {
                if (importee === '@lwc/renderer-abstract') {
                    return 'my-custom-renderer-implementation';
                }
            },
        },

Is this not compatible with your proposal?

[caridy]

Is this not compatible with your proposal?

No, because the compiled code has no idea what function to import, it can only import everything, and passing it around for core to do its thing.

[nolanlawson]

After talking with Caridy, I get it now. Each template needs its own sandbox. There's not just one sandbox for the whole engine JS file. So the template can't just import the renderer APIs from some shared module.

I could still imagine some clever way to compile away the boilerplate for the off-core usecase (e.g. maybe some reference to the sandbox could be passed in as the first argument to every renderer API), but I dunno if it's worth it for the 1-4% performance gain. Worth noodling on though.

[nolanlawson]

@caridy After chatting with @pmdartus, we have a couple counter-proposals:

1. Instead of attaching the renderer to each and every VNode, what if we attach it to the Template during the registerTemplate phase? This would avoid having to associate every VNode with the renderer, which should avoid a lot of vnode.renderer.*() lookups.
2. (My additional proposal) Rather than attaching the renderer to the Template, could we attach some object that is used to identify the Template for the purposes of sandboxing? This object would be created in registerTemplate, so it would be controlled by the engine, not the Template. Then we would pass this object into every renderer API, e.g. createElement(sandboxIdentifier: any, tagName: string). Then we would build a custom version of LWC in lwc-platform where Locker hooks into every renderer call. This would achieve the hoisting optimization, with the downside of needing to plumb the sandbox identifier through the engine.

[caridy]

@caridy After chatting with @pmdartus, we have a couple counter-proposals:

1. Instead of attaching the renderer to each and every VNode, what if we attach it to the Template during the registerTemplate phase? This would avoid having to associate every VNode with the renderer, which should avoid a lot of vnode.renderer.*() lookups.
2. (My additional proposal) Rather than attaching the renderer to the Template, could we attach some object that is used to identify the Template for the purposes of sandboxing? This object would be created in registerTemplate, so it would be controlled by the engine, not the Template. Then we would pass this object into every renderer API, e.g. createElement(sandboxIdentifier: any, tagName: string). Then we would build a custom version of LWC in lwc-platform where Locker hooks into every renderer call. This would achieve the hoisting optimization, with the downside of needing to plumb the sandbox identifier through the engine.

@nolanlawson neither of those will work with synthetic shadow or light-dom where each vnode might be associated to a different template at the time of the reification of the vnode into a dom node. What this means is that I might "render" a node into the DOM that was produced by a template two levels up, which might be running in a different sandbox, and it is subject to a different set of distortions/capabilities.

[pmdartus]

What this means is that I might "render" a node into the DOM that was produced by a template two levels up, which might be running in a different sandbox, and it is subject to a different set of distortions/capabilities.

Correct. Today, VNodes are passed from the parent to the child components in the Light DOM and synthetic shadow DOM. Currently, each VNode has to have enough information on itself to be rendered by the child component. It includes the owner with a back pointer to the owner VM to deal with the extra setup required for synthetic shadow DOM. And now with this PR, VNodes carry a pointer to the renderer object.

That said, I don't think it should be the case. All the VNodes produced by a given template share the same owner and same renderer. Since most of the VNodes in a given component are rendered in their owning shadow tree, all those VNodes will share the same values. Ideally, we should remove all those duplicate properties.
Instead of storing this information on each individual VNode, those properties could be stored for example on the VM. And instead of passing a list of VNode to the children component, the parent component could pass an object containing the owning VM and the renderer API as well. @jodarove has to an early property of this in #2716 (see AllocatedVNodes).

Granted it will make the engine code a little more complex but I see the following advantages in contextual information from the VNode:

• Making the VNodes more portable. It will also open the door for more complex hoisting capabilities.
• Reduce the memory footprint of each VNode.
• Smaller generated code size. This is especially true for this PR where now each VNode factory function expects a new argument.

[caridy]

@pmdartus I agreed with you! I have changed this PR to reflects that, now the renderer is bound to the VM, so no more changes in the shape of the vnodes.

[caridy]

@ravijayaramappa it turns out that this PR is very far from what we discussed today. It is about one renderer API per template (or the vm that holds the template to be more precise), but what we want is a renderer API associated to a vnode (and consequently to the reifying version of that which is the node).

[pmdartus]

What is the value of keeping around defaultRender? Since all the VM have an associated renderer, the renderer can be passed as an argument to the patching method: patchChildren and hydrateChildren.

[caridy]

I will like to explore that. You still need to have access to the defaultRenderer though

[caridy]

done something similar, instead of default renderer, I'm passing the proper renderer, whether it is default or not is relevant... take another look.

[pmdartus]

Nice.

Now speaking about the getRendererFromVNode. If we put aside all the usage of modules/*.ts, this function is only used in patch and mount. In both places, we have access to the owner's renderer. We could update the getRendererFromVNode renderer to completely get rid of this concept of default renderer:

export function getRendererFromVNode(vnode: VBaseElement, fallbackRenderer: RendererAPI): RendererAPI {
    return vnode.data.renderer ?? fallbackRenderer;
}

[caridy]

that will be awesome, the whole setDefault* injection mechanism, we know how that goes... so I believe you're absolutely right, because a vnode can't be created without creating a vm first. I will work on this today.

[caridy]

it is gone!

[caridy]

@pmdartus the reasoning for going with two abstract methods for collecting the renderer was to avoid mistakes. since the renderer is available off of the vm, and vm is accessible everywhere (including vnode.owner), to avoid mistakes (mostly destructing) we came up with the idea of using the methods instead. We can remove that abstraction for sure, that's what we had before.

[ravijayaramappa]

Still reviewing.

[ravijayaramappa]

This is great!

[ravijayaramappa]

You are right, the token comes from user land. The user can tinker with the compiled template and change the token. So it must be run through the hooks.

[nolanlawson]

Before we merge this, I'd like to have a conversation about whether the current renderer is the right level of abstraction to expose to Locker. It's the right level of abstraction for splitting up @lwc/engine-dom from @lwc/engine-server, but that doesn't necessarily mean it's the right level of abstraction for Locker.

Also I'd like us to run the perf tests just to get an idea of the impact of this PR. (yarn build:performance && yarn test:performance)

[nolanlawson]

This duplication is a bit unfortunate. Before #2598 we had a way to consolidate some of this logic:

lwc/packages/@lwc/engine-core/src/framework/base-lightning-element.ts

Lines 498 to 518 in caa3c72

	const childGetters: Array<
	[
	keyof HTMLElement,
	keyof Pick<
	Renderer,
	| 'getChildren'
	| 'getChildNodes'
	| 'getFirstChild'
	| 'getFirstElementChild'
	| 'getLastChild'
	| 'getLastElementChild'
	>
	]
	> = [
	['children', 'getChildren'],
	['childNodes', 'getChildNodes'],
	['firstChild', 'getFirstChild'],
	['firstElementChild', 'getFirstElementChild'],
	['lastChild', 'getLastChild'],
	['lastElementChild', 'getLastElementChild'],
	];

That said, I don't have a strong opinion, because gzip will remove a lot of the duplication, and maybe it's not worth all the TypeScript gymnastics.

[nolanlawson]

There is also similar duplication below for querySelector et al.

[caridy]

yeah, the main issue is the renaming of the methods... if we rename the methods in renderer.* then this becomes an easy step here, but that's not the case right now.

[caridy]

@nolanlawson these are the results... I'm not sure how to represet this from the CLI, but I can tell that most were faster for this PR, few were unsure, and none were slower.

| Benchmark                                          | Before (low) | Before (high) | Before (avg) | After (low) | After (high) | After (avg) | Delta (low) | Delta (high) | Delta (avg) | Delta perc (low) | Delta perc (high) | Delta perc (avg) |
| -------------------------------------------------- | ------------ | ------------- | ------------ | ----------- | ------------ | ----------- | ----------- | ------------ | ----------- | ---------------- | ----------------- | ---------------- |
| dom-light-styled-component-create-10k-same         | 155.49       | 157.87        | 156.68       | 159.15      | 160.50       | 159.82      | 1.77        | 4.51         | 3.14        | 0.01             | 0.03              | 0.02             |
| dom-light-styled-component-create-1k-different     | 135.64       | 137.60        | 136.62       | 135.96      | 138.10       | 137.03      | -1.05       | 1.86         | 0.41        | -0.01            | 0.01              | 0.00             |
| dom-ss-native-styled-component-create-10k-same     | 323.99       | 330.24        | 327.12       | 329.24      | 335.98       | 332.61      | 0.89        | 10.09        | 5.49        | 0.00             | 0.03              | 0.02             |
| dom-ss-native-styled-component-create-1k-different | 140.87       | 142.95        | 141.91       | 141.43      | 142.52       | 141.97      | -1.11       | 1.23         | 0.06        | -0.01            | 0.01              | 0.00             |
| dom-ss-slot-create-container-5k                    | 437.07       | 445.81        | 441.44       | 448.38      | 459.52       | 453.95      | 5.43        | 19.59        | 12.51       | 0.01             | 0.04              | 0.03             |
| dom-ss-slot-update-slotted-content-5k              | 290.35       | 297.79        | 294.07       | 294.17      | 301.36       | 297.77      | -1.48       | 8.87         | 3.70        | -0.01            | 0.03              | 0.01             |
| dom-ss-styled-component-create-10k-same            | 295.90       | 301.39        | 298.65       | 299.72      | 304.40       | 302.06      | -0.19       | 7.02         | 3.41        | -0.00            | 0.02              | 0.01             |
| dom-ss-styled-component-create-1k-different        | 148.49       | 149.54        | 149.01       | 148.57      | 150.18       | 149.38      | -0.60       | 1.33         | 0.36        | -0.00            | 0.01              | 0.00             |
| dom-styled-component-create-10k-same               | 281.16       | 282.57        | 281.87       | 285.01      | 286.87       | 285.94      | 2.90        | 5.24         | 4.07        | 0.01             | 0.02              | 0.01             |
| dom-styled-component-create-1k-different           | 141.20       | 142.65        | 141.92       | 142.12      | 142.89       | 142.51      | -0.23       | 1.40         | 0.58        | -0.00            | 0.01              | 0.00             |
| dom-table-create-10k                               | 186.42       | 188.50        | 187.46       | 188.66      | 191.18       | 189.92      | 0.83        | 4.09         | 2.46        | 0.00             | 0.02              | 0.01             |
| dom-tablecmp-append-1k                             | 52.99        | 54.49         | 53.74        | 54.38       | 56.11        | 55.25       | 0.37        | 2.65         | 1.51        | 0.01             | 0.05              | 0.03             |
| dom-tablecmp-create-10k                            | 419.96       | 430.32        | 425.14       | 431.36      | 448.22       | 439.79      | 4.75        | 24.55        | 14.65       | 0.01             | 0.06              | 0.03             |
| dom-tablecmp-create-1k                             | 69.39        | 72.10         | 70.75        | 72.14       | 75.58        | 73.86       | 0.92        | 5.31         | 3.11        | 0.01             | 0.08              | 0.04             |
| dom-tablecmp-hydrate-1k                            | 57.90        | 59.99         | 58.94        | 58.31       | 65.46        | 61.89       | -0.78       | 6.66         | 2.94        | -0.01            | 0.11              | 0.05             |
| dom-trackedcmp-create-10k                          | 385.62       | 396.61        | 391.11       | 397.38      | 409.97       | 403.68      | 4.21        | 20.92        | 12.57       | 0.01             | 0.05              | 0.03             |
| server-table-render-10k                            | 214.30       | 216.62        | 215.46       | 218.40      | 220.05       | 219.22      | 2.34        | 5.19         | 3.76        | 0.01             | 0.02              | 0.02             |
| server-tablecmp-render-10k                         | 217.30       | 219.58        | 218.44       | 217.01      | 220.05       | 218.53      | -1.81       | 1.99         | 0.09        | -0.01            | 0.01              | 0.00             |

[nolanlawson]

@caridy Yeah the markdown table is not super easy to read; I have a Google Sheet I use to format the results more nicely. Here's what your results look like:

So it's a regression, but in the 1-5% range. This is about what I expected. Thanks!

[nolanlawson]

Chatted with @caridy about the "right level of abstraction" issue. I think there are tradeoffs here (not having a tailor-made API surface for LWS vs potentially having tight coupling of the existing renderer API surface with downstream consumers like LWS), but given that we might have downstream consumers other than LWS, I'm fine with just exposing the whole renderer API surface. Just wanted to think a bit about the tradeoffs.

[nolanlawson]

Removing my block on the PR

[nolanlawson]

Approving to remove my block, I'd like to see Ravi approve too before merging ideally