Allow mixins to access custom element lifecycle

[jbenallen]

Is your feature request related to a problem? Please describe.
Mixins have no way to guarantee hooks into an element's lifecycle (connectedCallback, disconnectedCallback, renderedCallback). It is hindered in two ways.

First, if a mixin defines a connectedCallback method in its class body, a custom element that overrides the same method will prevent the mixin's implementation from being called. The custom element would need to call super.connectedCallback() in order to invoke the mixin's implementation. This has two problems. (1) The mixin can't guarantee or require that the custom element include this logic. (2) The custom element must know that the super includes that method because it is not included by default in the Element class. Otherwise, the custom element must conditionally check for it and call it just in case the mixin defines it. For example:

export default class MyElement extends Mixin(Element) {
  connectedCallback() {
    if(super.connectedCallback) super.connectedCallback();
  }
}

Second, if the mixin attempts to patch the instance in its constructor, attempting to set this.connectedCallback to another value, an error is thrown with this message: Cannot assign to read only property 'connectedCallback' of object '[object Object]'

export const Mixin = (Base) {
    return class extends base { 
        constructor() {
            super();
            const connectedCallback = this.connectedCallback;
            this.connectedCallback = () => { // Error is thrown here!
                console.info("mixin patch!!");
                connectedCallback();
            };
        }
    }
}

Describe the solution you'd like
I'm not entirely sure what the best solution should look like in detail. The options I've considered are included below. Option 2 below is probably my favorite given its ergonomics and support for extending web components from the outside without breaking any semantics.

Describe alternatives you've considered

Option 1)
Changing the prototype chain sequence resolves this if the mixins are applied to the custom class itself instead of applied to the base class, creating a new base class to extend.

Example, given this mixin:

const Mixin = (base) => {
    return class extends base {
        connectedCallback() {
            console.log("FIRST");
            if(super.connectedCallback) super.connectedCallback();
        }
    }
}

This works:

export default Mixin(class MyElement extends Element {
  connectedCallback() {
    console.log("SECOND");
  }
})

and this works:

class MyElement extends Element {
  connectedCallback() {
    console.log("SECOND");
  }
}
export default Mixin(MyElement);

This change in prototype chain works around the problem since it can be assumed that a mixin developer will be responsible enough to invoke the super.connectedCallback if it exists. However, it creates an awkward binding when the mixin provides methods on its prototype that the class calls from its own methods. It works in practice and runtime, however, the semantics don't match exactly.

This is not my favorite solution, but it works currently.

Option 2)
Expose a way to add listeners for these lifecycle events. External code can listen to them as events using its EventTarget APIs.

Example:

element.addEventListener('connected', listener); // non-bubbling event

This is non-standard, but renderedCallback is currently supported and isn't in the standard either.

Option 3)
Allow lifecycle methods (e.g. connectedCallback, disconnectedCallback, renderedCallback) to be writeable on the instance.

Option 4)
Add linting rules that require lifecycle event methods to call super.lifecycleMethod() in their implementations. This would also require that Element has a default implementation to avoid errors. This is a breaking change.

[caridy]

Yeah, I suspected that at some point this was going to be a problem. At the moment, we just support the bare bone API from WC, where HTMLElement does not have those methods, so, invoking a super without the brand check throws. E.g.:

    class Base extends HTMLElement {
        connectedCallback() {
            // super.connectedCallback(); // throws
            console.log('base connected');
        }
        disconnectedCallback() {
            // super.disconnectedCallback(); // throws
            console.log('base disconnected');
        }
    }
    class Foo extends Base {
        connectedCallback() {
            super.connectedCallback();
            console.log('foo connected');
        }
        disconnectedCallback() {
            super.disconnectedCallback();
            console.log('foo disconnected');
        }
    }

So, there are few things:

• LightningElement is our abstraction layer on top of HTMLElement, we could very well solve this by adding the default behavior. I was against this in the past due to perf implications, but I have some ideas how to opt out when needed. This will eliminate the brand check in extended classes.
• WC semantics dictates that the hooks are extracted during registration from the prototype, instead of getting them from the instance. Changing that behavior is a no-go for us, it is also a huge perf optimization.
• Services are in place today to provide access to those hooks globally for all instances created, but I really really really want to remove services, they can't be used arbitrarily without introducing a bug perf problem.
• option 1 makes it more difficult for us to statically analyze the class, which is a big problem.
• option 2 is something that will be possible in the future once the mutation observer can observe for those hooks, hard to do it at the moment. We should double check how is that spec progressing.
• option 3 is a problem due to the details above, it changes the semantics, but let me think more about it.
• option 4 seems to be the leading candidate for me. The only problem is when you DO NOT want to execute the super logic, because you want a brand new logic. This also raises the question of what happen if you mix the same thing more than once in the proto chain? Do we error? do we opt-out on some pieces? who controls this?

Users of the Mixins should be aware that they are sugar layer for class inheritance, and if some of them requires them to call the super, and take care of some of the work. Also, mixins are probably on the 20 rather than on the 80, so, it might be fine. Let's debate more about this.

[jbenallen]

Option 4 seems completely valid to me. Requiring the invocation of each constructor in the prototype chain resolves a class of problems which surface again here, specifically that any inherited class in the chain cannot function correctly without its constructor being called. The same holds true for base classes that use the lifecycle events. For example, one may need to clean up references when the element is disconnected. Without a way to guarantee disconnection cleanup, memory leaks will ensue for these cases and there will be no way to ensure they can't if the user writes bad code and doesn't call the super.

The wire decorator has access to connected/disconnected lifecycle events, so there needs to be a way to do this for @wire to be a real decorator instead of having a secret handshake with the engine just to function correctly.

[diervo]

Let me re-read this and shall we have a meeting tomorrow to align since @pmdartus is back?

[pmdartus]

I also think that option 4, it the best approach.

Add linting rules that require lifecycle event methods to call super.lifecycleMethod() in their implementations.

As @caridy pointed out, Mixin represent only the 20 of the 80/20. I would like to only restrict this warning to all the classes that doesn't inherit directly from the base LightningElement.

[caridy]

After a failed attempted to implement 4, I can assure that this option is not really an option. Mostly because you don't have an opt out mechanism, and in many case, what you want is to NOT call the super. E.g.: if I have my own template, and I'm extending a component that is doing some work on its dom in renderedCallback, I don't want their code to run, because it will NOT work since the dom is very different. This is the same problem exhibit by option 1.

I was tempted to try to implement the event, which at first seems simple enough and also could simplify the services (to avoid calling the service on every element when inserted), but that had a similar problem, people might decide to just stopImmediatePropagation() and we will have the reserve problem, the super will execute, but the child will not because the dispatching order is inverse. So, option 2 is also a no-go for those reasons.

Option 3 is also out because those hooks are extracted at the constructor level by spec, they are not picked from the instance. Btw, they are configurable, so in theory you could override them by using defineProperty on the instance, but that will not work due to the extraction mechanism. Also, the ergonomic of this for Mixin authors will be pretty bad. So, I will not consider this one an option.

So, we are back to the whiteboard again :(

[davidturissini]

I'd like to challenge the assumption that mixins should be concerned with lifecycle methods at all. The point of writing a mixin is to provide shared functionality to multiple components. This functionality should not be coupled to custom element lifecycle methods and I would go so far as to suggest that this is an anti-pattern. Coupling to lifecycle methods in mixins makes the mixin very brittle and far less flexible. It should be up to the component author to decide when to invoke that functionality.

For instance, if I have a drag and drop mixin that uses lifecycle hooks, I may have something like this:

class DragAndDropMixin {
  connectedCallback() {
    this.addEventListener('dragstart', (evt) => {
       // do something
    })

   this.addEventListener('drag', (evt) => {
       // do something
    })

    this.addEventListener('dragend', (evt) => {
       // do something
    })
  }

  disconnectedCallback() {
    this.removeEventListener('dragstart')
    this.removeEventListener('drag')
    this.removeEventListener('dragend')
  }
  
}

To consume this mixin, I might do something like this:

class MyCmp extends Mixin(DragAndDropMixin) {
  connectedCallback() {
    // component logic
    super.connectedCallback(); // drag and drop
  }

  disconnectedCallback() {
    // component logic
    super.disconnectedCallback(); // unbind drag and drop
  }
}

What happens if I want the drag and drop functionality not to happen on connect, but after some user interaction? Then I have to write something very awkward:

class MyCmp extends Mixin(DragAndDropMixin) {
  handleButtonClick() {
     super.connectedCallback(); // connectedCallback here to enable drag and drop?
   }
}

Instead of using lifecycle methods, mixing should instead provide methods that users can invoke whenever they want:

class DragAndDropMixin {
  startDrag() {
    this.addEventListener('dragstart', (evt) => {
       // do something
    })

   this.addEventListener('drag', (evt) => {
       // do something
    })

    this.addEventListener('dragend', (evt) => {
       // do something
    })
  }

  endDrag() {
    this.removeEventListener('dragstart')
    this.removeEventListener('drag')
    this.removeEventListener('dragend')
  }
  
}

If we prevent mixins from using lifecycle methods, I don't see how this issue is a problem anymore

[jbenallen]

The main reason for wanting this support is that it reduces how much is pushed back to individual component developers. We all know by years of experience that developers do the wrong thing on accident regularly, and forgetting or not knowing that they are supposed to add code is a common problem. This is particularly true if the code works functionally and there are no warnings or errors that prompt them that something is wrong.

For instance, the developer of a mixin that registers a listener for a given component in a shared map will have the ability to ensure that memory leaks will not occur if the developer of the consuming component does not explicitly call another mixin API to disconnect them. The mixin developer is aware of the memory implications and for the need to clean it up. The component developer may not be aware of that because the way the memory is allocated and consumed is abstracted and opaque behind the mixin's API. The mixin developer is also unable to guarantee or enforce that its consumers call the disconnecting API. If a memory leak does occur, it will be associated to the mixin given the memory allocation even though the mixin is not responsible and has no power to fix it.

The framework, on the other hand, doesn't require DOM event handlers to be removed as it is taken care of automatically. This is true for @wire decorators as well. Everything is automatically cleaned up because of privileged hooks into the lifecycle. My hope has been that we can provide mixin developers the ability to provide this same level of support so that component developers have a consistent mental model and don't accidentally break the entire app without knowing otherwise.

[caridy]

I will argue that the problems you are describing are not really about mixin but inheritance is general. I might have a subclass that doesn't honor the super hooks, because the super doesn't have them defined, but then they added it, and now my subclass is broken.

[caridy]

Is there anything else here @jbenallen @davidturissini ? Can we close this? It feels to me that we explored most options, and this will remain as it is (status-quo).

[jbenallen]

Circling back to this. In general, this is a reasonable limitation. However, I want to point out that the @wire decorator implementation already requires a callback when the element is disconnected from the document. It currently works using a privileged service which is baked into the system and is not public. Without the engine providing a public means to subscribe to an element's disconnected lifecycle event, the @wire decorator will always require a private hook and can never be extracted out of the private core. Given this, I see two possible solutions:

1. Create a public API on the LightningElement that supports this. This thread has largely circled around this option.
2. Allow a mixins to leverage whatever mechanism the @wire decorator uses to be notified when the element is disconnected, even if this is something like a service.

@caridy

[caridy]

@jbenallen I believe this should be possible via #899, we just need to validate that assumption.

[caridy]

@ravijayaramappa this one could be solved by #1431

[ekashida]

@caridy @jbenallen I believe we've since started moving away from mixins. Should we close this one?

[caridy]

I don't know... I am up for closing it... but I don' tknow if @jbenallen or someone else might have strong opinions here.