rfc: engine plugins

[davidturissini]

Details

RFC for LWC plugins

Does this PR introduce a breaking change?

• Yes
• No

[kevinv11n]

Typo in creation. Also this hook fires post constructor pre prop assignment.

[kevinv11n]

wire doesn't use this

[kevinv11n]

typo is likewise

[pmdartus]

Suggested change

	```
	```js

[pmdartus]

Suggested change

	```
	```js

[pmdartus]

Suggested change

	targetContext : isFunction(locatorContext.context) && locatorContext.context(),
	targetContext : typeof locatorContext.context === 'function' && locatorContext.context(),

[pmdartus]

Suggested change

	hostContext : isFunction(hostElement.context) && hostElement.context()
	hostContext : typeof hostElement.context === function && hostElement.context()

[pmdartus]

IMO plugins is considered as trusted code, I don't see any issue with giving the actual object instead of a facade.

[caridy]

yeah, today we give them the component instance, which is supposed to have less capabilities than the element, although in reality, from the component you can always access the element via this.template.host, which was blocked in the pass. My fears were mostly about wire adapters doing funky stuff by using the DOM as a transport layer, but for that we do have another layer, the wire decorator itself, so I think we will be fine exposing both.

[pmdartus]

I don't understand how this plugin would work.

If an element does not have locator data on its context, the locator protocol is never invoked.

When we want to profile a component, we would need to a to all components the benchmark flag in their context?

[davidturissini]

This is an open question I have as well. Is context a requirement for executing a plugin or should the engine ask if the plugin should be executed for a particular element?

Potentially, we could add a shouldInvokePlugin method (we can bikeshed on name) that tells the engine whether a plugin should be invoked for a given element:

shouldInvokePlugin(element: HTMLElement, componentInstance?: ComponentInstance) -> Bool

[tariqrafique]

benchmark could be one of those categories of plugins that you would want to run for all elements. We shouldn't need components to be tagged with a benchmark attribute

[caridy]

This is not a problem. Remember, you can add metadata to all elements if you can associate that to LightningElement. Similarly, the compiler can add metadata to any class via registerComponent() to instrument all or a subset of them, saying, I want to instrument only lightning namespace components.

[tariqrafique]

So if I choose to, at runtime I can make a plugin run for all components ?

[caridy]

yes, sort of... you must have accesss to the plugin, but we haven't really get into that just yet.

[pmdartus]

I don't understand what is hooks applied to all the elements rendered by the engine, while others are used only on components. For example, I would expect elementWillRender to be only applied to the element associated with a component.

[caridy]

this is a good question. I have a similar reaction, and I think we can change the name to something like elementWasMutated, which for custom elements means that it is (or might be) about to be rehydrated/rendered, and for a regular element it means that it was just mutated, nothing else.

[caridy]

I think we will need a reflection for this one, elementWillMutate and elementDidMutate, so, wire and other services can prepare ahead of time.

[davidturissini]

I agree, this hook can be limited to only custom elements. @caridy when do you envision elementDidMutate firing? Before a component has rendered but after props have been applied?

[caridy]

no, not related to rendering because regular elements don't have this. this is about I'm about to diff this element, vs I'm done diffing the element.

[pmdartus]

That's a good question, I think there is an overlap here with my comments about the APIs. Some of the APIs today feels that they are only applied to components and not to elements.

[caridy]

well, the hook is really about creation, which is the process of creating the element, whatever that element isn't or not is not that important.

[davidturissini]

The motivation for this was around benchmark, which only cares about the timing of Custom Elements constructor, not native elements

[pmdartus]

As far as I understand this proposal the only way to add context to an element is via the template. There is theoretically no way to add new plugins without instructing the compiler. Right?

[caridy]

@pmdartus no, this proposal contemplates decoration of the class definition to add metadata to the component instance, so it is analogous to adding the same info via a template onto instances of this component.

As for the note, I don't think we want to differentiate them, but we can chat more about this.

[caridy]

just a clarification, with this change, the wire service can be only one... you can't call register with wire more than once, is that correct?

[caridy]

if the answer to my question is yes, then that means you can always call register('wire', {}) to disable that service for testing and other scenarios that are not possible today.

[davidturissini]

Yea, the idea here is that you should only call register with a given identifier once. Essentially, this will add the plugin to an internal map. They identifier would be used to grab the context data and pass it to the plugin.

[caridy]

If we ever start using this for internal engine job (internal plugins), we need to guarantee that they can't be overrule by another one, maybe we can achieve this with some sort of aggregation, e.g.: assign(create(null), externalPlugins, internalPlugins);

this also pose another restriction, two independent pieces of code can't really actuate over one plugin identifier, but I think that's a cleaner design overall.

[caridy]

one open question here is what to do when both, the class context and the template content for instances of that class are defined for a service, which one wins? or is this aggregated? or is this provided to the service somehow? I don't have a specific scenario where that is a thing, but could be possible for other services in the future.

[davidturissini]

I'll need to think about this some more, but I think one scenario where this might happen would be with wire. IIRC, there was a wire directive at some point for query rollup. Not sure if thats still the plan, but this is definitely something to think about.

[davidturissini]

One way we could approach this, is to have the plugin itself handle merging data. For example, if plugin foo has class context and template context, we could ask foo to generate a single context object from that. That way, the plugin can decide which one wins.

[caridy]

I will like to see more details about this.

[caridy]

more folks that should review this:

• @tariq-sfdc
• @diervo
• @jbenallen

[tariqrafique]

One of the nice things we have today is that locators can get some sanity checking today with the compiler. locator:id must be a string and locator:context must be an expression. Additional this locator:context with MemberExpression is also blocked.

<a ... locator:context={foo.bar}>

The reason for this is that the user shouldn't have any expectation that we will execute the context function callback within non-component context. The locator:context function is always executed within the Component this context.

You can see some examples of parser validation here

[tariqrafique]

We'll need make a decision at compile time on what context we would expect plugin attribute expressions to be bound to.

<template for:each={state.todos} for:item="todo" for:index="index">
    <li key={todo.id}>
        <button onclick={todo.clickHandler}
                locator:id="todo-item" locator:context={locatorProviderTodo}>
            {todo.text}
        </button>
    </li>
</template>

In the example above, locatorProviderTodo is bound to the component instance

if instead, it was written to use a function from the todo item in the iteration,

<template for:each={state.todos} for:item="todo" for:index="index">
    <li key={todo.id}>
        <button onclick={todo.clickHandler}
                locator:id="todo-item" locator:context={todo.locatorProviderTodoItem}>
            {todo.text}
        </button>
    </li>
</template>

what should the function bind to ? probably the todo item. or maybe not.

To avoid this decision, we enforce that for locators, you can only use context function that's directly on the component and it will only be bound to the component at invoke time. Anything with a member expression is blocked

[caridy]

Yes, I think schema validation for custom directives are not that hard to support, it is just a configuration option in the compiler IMO, @pmdartus will know better.

[tariqrafique]

for locatorContext.context() to execute within with the Component this context, we'll need to make sure that the compiler always binds any plugin expression functions to the component.

[caridy]

this is an easy fix, we have that in the scope.

[tariqrafique]

Suggested change

	};
	};
	handler(evt);

[diervo]

I overall like the proposal to unify context, however I'm concerned with giving access to pretty much all internals. I would rather be opening this slowly and with abstractions rather than give access to all things.

Moreover it seems that we will have way too many hooking points, which may have some bad perf impact plus might be confusing. We should try to figure out if we can boile down both the hooks and the arguments being passed in.

[davidturissini]

Closing for now. If we need to, we can reopen