fix(template-compiler): support custom renderer hooks for external components #3559
Conversation
| @@ -46,7 +46,12 @@ export interface CustomRendererConfig { | |||
| } | |||
|
|
|||
| function checkElement(element: BaseElement, state: State): boolean { | |||
| // Custom elements are not allowed to have a custom renderer hook. | |||
| // Elements of type `ExternalComponent` may have custom renderer hooks. | |||
| if (state.crDirectives.has('lwc:external') && element.type === 'ExternalComponent') { | |||
There was a problem hiding this comment.
Does element.directives not have 'lwc:external' when element.type === 'ExternalComponent'?
There was a problem hiding this comment.
Yeah, we had decided to create a new ExternalComponent type instead of passing the directive around in anticipation of out-of-bound configurations.
| // Custom elements are not allowed to have a custom renderer hook. | ||
| // Elements of type `ExternalComponent` may have custom renderer hooks. | ||
| if (state.crDirectives.has('lwc:external') && element.type === 'ExternalComponent') { | ||
| return true; |
There was a problem hiding this comment.
I'm worry about the next condition, which returns false for dynamic components. Does true here means that it is risky? or that this should be treated as a result element? This method on itself is confusing
There was a problem hiding this comment.
This method is used to determine whether or not an element should get custom renderer hooks, so yes, returning true here marks it as "risky".
Could you elaborate your concern about dynamic components? I don't believe we support lwc:external for those, and the existing comments seem to imply that custom elements of LWC components should never need custom renderer hooks.
There was a problem hiding this comment.
@ekashida Can you find a suitable name for this method and update it in master branch? 🙏
checkIfElementisRisky or isElementRIsky or doesElementRequireRenderer are some ideas.
Details
Fix to support custom renderer hooks for components of type
ExternalComponent.Does this pull request introduce a breaking change?
Yes, but only for a beta feature where LWS in enabled.
GUS work item
W-13555180