fix(overlays): allow Subclassers to teardown at the right moment

[tlouisse]

What I did

[changeset-bot]

🦋 Changeset detected

Latest commit: a2d8a73

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@lion/ui	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[ryubro]

Would it be possible to run the init callback when connected instead of firstUpdated? I find such symmetry (setting up when connected and tearing down when disconnected) would be cleaner than conditionally tearing things down.

[tlouisse]

Would it be possible to run the init callback when connected instead of firstUpdated? I find such symmetry (setting up when connected and tearing down when disconnected) would be cleaner than conditionally tearing things down.

Hey, good idea. We can probably go back to connectedCallback that waits until first update is completed.
Normally that is the right moment, since the render has happened by then.
Delaying the teardown is a sensible thing imho that we can keep, because it allows to move a dom node around w/o tearing down and setting up everything again.

See the updated code and especially the tests.

[tlouisse]

Would it be possible to run the init callback when connected instead of firstUpdated?

What do you mean by the init callback? We could indeed reduce some overhead by doing smth like this in a base class or mixin (named the callbacks setup/teardown, since we already use these ):

Lion base class:

    #hasSetup = false;

    connectedCallback() {
      super.connectedCallback();

      this.updateComplete.then(() => {
        if (this.#hasSetup) return;
        this.setup();
        this.#hasSetup = true;
      });
      this.#hasSetup = true;
    }

    async disconnectedCallback() {
      super.disconnectedCallback();

      if (await this._isPermanentlyDisconnected()) {
        this.teardown();
        this.#hasSetup = false;
      }
    }
    
    /**
     * When we're moving around in dom, disconnectedCallback gets called.
     * Before we decide to teardown, let's wait to see if we were not just moving nodes around.
     * @return {Promise<boolean>}
     */
    async _isPermanentlyDisconnected() {
      await this.updateComplete;
      return !this.isConnected;
    }
    
    /**
     * Here we are sure all dom (shadow dom and light dom) has rendered.
     * We can add event listeners to internal (and/or external) dom and perform related setup logic.
     * Also, we do not unnecessarily delay the first lit paint (which is vital for (perceived) performance),
     * that we would have otherwise done when setup logic would have been executed in connectedCallback.
     */
    setup() {
    }
    
    /**
     * Here we are sure that a component is permanently disconnected.
     * (instead of being moved around in dom, which synchronously triggers disconnected and connectedCallback).
     * When dom nodes are just being moved around, we don't unnecessarily hinder performance (INP).
     * We make sure that all event listeners are removed (especially the external ones, as they won't be automatically cleaned up).
     */
    teardown() {
    }

In subclassers, we would just use the setup/teardown callbacks
I'm all for it :)
(making things more bullet proof and align within all our components).

[ryubro]

@tlouisse

With init I meant this:

lion/packages/ui/components/overlays/src/configurations/visibility-trigger-partials/withClickInteraction.js

Line 23 in fef94cd

init: () => {

This callback is called in a very complex chain: OverlayController.constructor -> this._init() -> this._handleFeatures({ phase: 'init' }) -> this.__visibilityTriggerHandler[phase]() (I don't remember where firstUpdated came from. Maybe I meant constructor and my mind malfunctioned 🤪).

This new interface looks pretty straight-forward!! Do you also plan to simplify the overlay config?

[ryubro]

I was wrong about another thing during the communication. The bug that the teardown callback was called while the element was still on the memory didn't happen due to tabs, but due to a routing library. I believe the router detached the element but kept the reference to the element object hence the next time it was attached to the document again, the constructor was not called. In this scenario, !this.isConnected; would still yield true and the teardown() will be called. #hasSetup would be still true (because the instance wouldn't get recreated) and will have the same bug.

I still think that it would be much simpler to make sure that what is called during disconnected lifecycle callback has its pair in the connected lifecycle callback and be called, without any added state / conditions like hasSetup or isPermanatlyDisconnected. For instance, we can detach and attach again all the event listeners during the DOM is moving around. We might have a slight overhead but wouldn't be significant enough, I believe.

[tlouisse]

I was wrong about another thing during the communication. The bug that the teardown callback was called while the element was still on the memory didn't happen due to tabs, but due to a routing library. I believe the router detached the element but kept the reference to the element object hence the next time it was attached to the document again, the constructor was not called. In this scenario, !this.isConnected; would still yield true and the teardown() will be called. #hasSetup would be still true (because the instance wouldn't get recreated) and will have the same bug.

I still think that it would be much simpler to make sure that what is called during disconnected lifecycle callback has its pair in the connected lifecycle callback and be called, without any added state / conditions like hasSetup or isPermanatlyDisconnected. For instance, we can detach and attach again all the event listeners during the DOM is moving around. We might have a slight overhead but wouldn't be significant enough, I believe.

Imho after teardown #hasSetup would be false again (see code above) and everything would work.

Would you mind to make a failing test case for this @ryubro ?

The reason we would need to delay the setup is that LitElement didn't render everything yet at connected. Waiting for the first update after connected makes it on par with firstUpdated (https://lit.dev/docs/components/lifecycle/#firstupdated), but also allowing re-attachment.
The wait for an updateComplete in disconnected seems to work well when moving nodes around (no unneeded teardown + setup).
As long as we can abstract away the implementation details, it would be the most solid and performant solution imho.

[tlouisse]

This new interface looks pretty straight-forward!! Do you also plan to simplify the overlay config?

It's a great plan indeed. I made a branch like ~4 years ago, with a more readable and modular (separate files for all sub functionality, allowing to load large functionality with dynamic imports (like we do with Popper right now) and treeshaking).

But a new proposal is very welcome :) Just like a plan of how we can leverage popover + anchor positioning in a non breaking way

[ryubro]

Would you mind to make a failing test case for this @ryubro ?

Sure!