8000 fix(elements): allow custom elements to be detached and reattached by lmartorella · Pull Request #58586 · angular/angular · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

fix(elements): allow custom elements to be detached and reattached #58586

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

fix(elements): allow custom elements to be detached and reattached #58586

wants to merge 2 commits into from

Conversation

lmartorella
Copy link

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.dev application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

The commit fixes the issue #38778, allowing a HTML custom element defined via angular/elements to be disconnected from the DOM for indefinite time and then re-connected without breaking it.

The current behavior (correctly) destroys the Angular component after 10ms of grace time after receiving the disconnected event. This event can be called if the element is directly disconnected from his parent, or even when a larger DOM tree is disconnected and "parked" in memory for future reattach.

Since the mission of the Angular custom element support is to adhere to the W3C standards and write completely reusable Angular component for interoperability with other frameworks, it can be expected that detached DOM structures that contains a Angular element would be working even after reattaching the tree later on.

Destroying the Angular component when the HTML element is disconnected is correct, since it correctly frees all the resources and allow GC reclaiming of the orphan node.

However the current behavior of the componentRef.destroy() is to detach the Element node from his parent. This causes the issue, since the whole tree - when re-connected back later on - won't contain the custom element anymore, causing the issue.

Simply letting the NgElementImpl instance to remain attached to his parent mainly fixes the issue of the missing connected callback not called.

Rather than trying to allow a "non-detaching" version of destroy(), the proposed fix simply reattach the HTML Custom Element instance back to the DOM in the original position immediately after the componentRef.destroy(). This limits the fix to the elements package only. I'm not sure if there are some internal utilities to save/restore a node position in the DOM tree, so I used low-level DOM calls to do that.

          // Save old position of the element node, to maintain it attached even
          // after the this.componentRef.destroy, that detach it from parent
          const attachInfo = this.getElementAttachPoint();

          this.componentRef.destroy();
          this.componentRef = null;

          // Reattach the destroyed empty html element to the parent
          if (attachInfo) {
            const { parent, viewNode, beforeOf } = attachInfo;
            parent.insertBefore(viewNode, beforeOf);
         }

When the node will receive the "connected" event again later, the Angular component should be able to be recreated again from scratch, using the element properties and attributes, exactly like it was the first initialization. The extensive unit test suite already in elements is clear about the intent of element reuse, so the fix looks legit.

However, an additional fix is required to let the custom element to not lose the property values. The hosting framework will probably expect the custom element properties to remain in sync, considering the HTML Element implementation as a black box. This is something probably not fully documented in the W3C specs, but when disconnected a custom element should ideally retain his public property values like any other HTMLElement instance.

For that, I added a function to copy back input property values from the Component instance to initialInputValues immediately before the destruction. I foresee some possible issue about the inputs that uses the transform callback.

  private resetProperties() {
    this.componentFactory.inputs.forEach(input => {
      this.initialInputValues.set(input.propName, this.componentRef!.instance[input.propName]);
    })
  }

I don't see any inverted transform function available for inputs, so if we want 100% be sure that the properties are stored back properly the only working solution is to keep the initialInputValues set always in sync with the passed values (the risk in that case is to use more memory for properties that are supposed to only store volatile data decoded on-the-fly by the input).

Last line changed is in platform-browser, unfortunately, since the ShadowDomRendered must cope with the Element reuse in case of later reconnection. It seems that the W3C specs don't allow shadowRoot to be destroyed and then recreated, so the attachShadow would raise an exception if called on a element with already a shadow root present.

The proposed fix there requires a clear of the node content in case of reuse, otherwise duplicated nodes can be created (at least this happened in my tests).

    if (!this.shadowRoot) {
      this.shadowRoot = (hostEl as Element).attachShadow({mode: 'open'});
    } else {
      // In case of custom elements, it is possible that the host element was already initialized during the
      // element life-cycle (after a disconnect/reconnect)
      this.shadowRoot.innerHTML = "";
    }

I've added an additional integration test reconnect_spec.ts to test the following cases:

  • Direct disconnect/reconnect of the custom element node
  • Indirect disconnect/reconnect via disconnection from parent node
  • Test of components with shadow DOM and slots.

Does this PR introduce a breaking change?

  • Yes
  • No
  • Not sure

I don't know how many Angular features can break this fix, since a complete coverage for Angular components should be tested in the elements module.

However I don't expect regressions, since the current issue doesn't allow the nodes to be reused at all at the moment.

Different fix approaches

In order to avoid potential regressions due to the change, the fix could be introduced in different ways:

Setting

It should be possible to enable/disable such behavior via some setting exposed in the NgElementConfig. This would require some little documentation of the current behavior and the new one, to allow backward compatibility.

Inheritance

The current public NgElementStrategy interface is more or less enough to implement an alternative version of ComponentNgElementStrategy with the fix, directly in the app.
However this requires ~500 LOC to be copy/pasted in the app, and this is obviously subject to future fixes/evolutions, due to fact that these classes are mainly the core of angular/elements.

A viable intermediate solution is to expose the ComponentNgElementStrategy in public API as default implementation, to allow:

  • override of the destroy behavior
  • or exposing flags.

However this solution will require - again - quite extensive documentation of the current internal behavior.

Thanks, L

@angular-robot angular-robot bot added the area: elements Issues related to Angular Elements label Nov 11, 2024
@ngbot ngbot bot added this to the Backlog milestone Nov 11, 2024
@JeanMeche
Copy link
Member

I'm closing / reopening to trigger the tests.

@JeanMeche JeanMeche closed this Mar 4, 2025
@JeanMeche JeanMeche reopened this Mar 4, 2025
@thePunderWoman thePunderWoman marked this pull request as draft May 21, 2025 15:30
@thePunderWoman thePunderWoman removed the request for review from pkozlowski-opensource June 3, 2025 11:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
area: elements Issues related to Angular Elements
Projects
None yet
Milestone
Backlog
Development

Successfully merging this pull request may close these issues.
2 participants
@lmartorella @JeanMeche
0