angular: Angular Element is not removed and readded to the DOM properly

šŸž bug report

Affected Package

The issue is caused by package @angular/elements

Is this a regression?

No

Description

When an Angular Element is detached from the DOM and then re-attached, none of the event bindings work (see minimal reproduction for demo). I am a Google engineer and this is causing issues for us (internal bug filed here: http://b/168062117).

I talked to @kseamon about this and he thinks the issue is located in either of these 2 places:

https://github.com/angular/angular/blob/a69507a0adc0e559d152eb3fb988a410ef9352a7/packages/elements/src/create-custom-element.ts#L219

https://github.com/angular/angular/blob/a69507a0adc0e559d152eb3fb988a410ef9352a7/packages/elements/src/component-factory-strategy.ts#L109-L125

Specifically, this.scheduledDestroyFn in not set to null in the scheduled destroy and so may be called again upon connect. Iā€™m testing adding one line to see if it fixes the immediate issue.

However, even with this fix, there will still exist a tiny race condition that occurs when an element is scheduled to be destroyed and then connects before the destroy executes.

šŸ”¬ Minimal Reproduction

https://stackblitz.com/edit/angular-ovchwr?file=src%2Findex.html

šŸ”„ Exception or Error

No user-visible exception

šŸŒ Your Environment

Angular Version:

Iā€™m running it against the latest Angular in google3.

About this issue

  • Original URL
  • State: open
  • Created 4 years ago
  • Reactions: 4
  • Comments: 24 (11 by maintainers)

Most upvoted comments

Hi i have the same problem adding to angular js legacy app, ng-if breaks the angular web component.

To make sure we are on the same page, here is what the intention is with the current implementation:

  • When the custom element is removed from the DOM, the underlying component is scheduled for destruction after a short timeout (currently 10ms).
  • If the custom element is inserted back into the DOM before the timeout expires, then the component destruction is canceled and we keep the same component instance.
  • If the custom element is inserted back into the DOM after the timeout, then the component has been destroyed and we should create a new component instance.

This last bullet point is what we are failing to do at the moment, as mentioned in the previous comment (which is a bug), but that was the intention.

The reason we destroy the component instance is that there is some Angular-specific cleaning up that needs to happen when the element (and thus the underlying component instance) is no longer necessary (i.e. removed from the DOM for good).

The reason we have the short delay between removing the custom element from the DOM and destroying the component instance is to account for cases where you want to ā€œmoveā€ the element around (i.e. remove it from its current location in the DOM and immediately insert it at a new location). In that case, it would be a waste to destroy the component synchronously, only to re-instantiate it right after.

From a quick look, it seems that fixing the current bug (i.e. allowing the component to be re-instantiated when the host element is re-inserted into the DOM) is not trivial. For example, how do we handle content projection. The original nodes have been extracted from the host element the first time, so they are no longer there for us to extract. (We could cache them and re-use the same nodes, but I wouldnā€™t be surprized if this broke in some edge cases.)

Even if we managed to fix this and allow the component to be re-instantiated, the internal state of the original component instance would not have been retained. This would lead to unexpected UI differences.

The main problem here is that there is no good way to automatically infer whether a removed element is intended to be re-used some time in the future (and thus the underlying component instance should be retained) or whether it is not needed any more (and thus the underlying component instance should be destroyed).

Therefore, we have to resort to unreliable heuristics (ā€œif you havenā€™t added it back after 10ms you probably donā€™t need itā€).

An alternative approach would be to provide a way for people to denote that they intend to re-use a particular custom element and therefore the underlying component instance should not be destroyed after the element has been removed from the DOM. Something like:

myCustomElement.remove();

// I might reuse this in the future. Do not destroy the underlying component.
myCustomElement.ngRetainComponentInstance();

As a work-around, you can basically do this manually today (by using some private properties šŸ˜‡):

myCustomElement.remove();

myCustomElement.ngElementStrategy.scheduledDestroyFn()
myCustomElement.ngElementStrategy.scheduledDestroyFn = null;

https://github.com/angular/angular/issues/38778#issuecomment-697931683

The work-around at the bottom of https://github.com/angular/angular/issues/38778#issuecomment-690433315 does not work for us for 2 reasons: (1) the component is destroyed by the @angular/animations package (Iā€™ve filed https://github.com/angular/angular/issues/38962 for that) and (2) the inputs are not reinitialized from the HTML attributes when it reconnects.

I also bumped into this recently, we found that using a setter seems to give consistent results when trying to assign values from inputs on any subsequent initializations of an angular/elements web component.


private _myValue = '';

@Input()
set myValue(value: string) {
  if (value) {
    this._myValue = value;
  }
}

The caveat is that the setters seem to run after the child component loaded in the router-outlet tries to access the value, which was solved with a boolean thatā€™s set to true in the afterViewInit function.

Something like this in the AppComponent:

<div *ngIf="isInit">
 <router-outlet></router-outlet>
</div>
public isInit = false;

// allows the setters to execute
ngAfterViewInit(): void {
 this.isInit = true;
}

Doesnā€™t solve the root issue, but has unblocked our work thus far.

This bug is so severe that basically prevents Angular Elements from being used for developing a component library, unless developers are asked to avoid detaching and reattaching elements - which is a totally ludicrous request.

Iā€™ve fiddled around this problem using a custom NgElementStrategyFactory class, basically copying the original ComponentNgElementStrategyFactory (this is not good DX, folksā€¦ I hope itā€™ll get there) but removing the return in the connect method (also references to Zone.js). That seemed fine and the component would get recreated, butā€¦ then it crashes as soon as ShadowDomRenderer tries to reattach the Shadow DOM without checking if thereā€™s one already (note: this is also incompatible with Declarative Shadow DOM) and eventually reusing or removing its content.

I donā€™t know if itā€™s worth considering making a PR out of this, since many other things are out of my comprehension of the problem, but let me know. I havenā€™t even checked if the other encapsulation modes are fine.

Anyway, Iā€™d like to say that itā€™s natural to compare the life cycle of Web Components to the one of Angular components, so that ngOnInit/ngAfterViewInit are basically the equivalent of connectedCallback, while ngOnDestroy is basically disconnectedCallback. As developers, I think we need to rely on this.

The work-around at the bottom of https://github.com/angular/angular/issues/38778#issuecomment-690433315 does not work for us for 2 reasons: (1) the component is destroyed by the @angular/animations package (Iā€™ve filed https://github.com/angular/angular/issues/38962 for that) and (2) the inputs are not reinitialized from the HTML attributes when it reconnects. Furthermore, we would prefer to not couple Angular Element removal logic with our host application.

My main goal when filing this bug was to see if we could get removing and re-adding an Angular Element to work. I donā€™t mind if the element needs to re-render or reinitialize. I solely want the host application to be capable of removing and re-adding the element and the element to be in the correct state when re-added.

[Deleted comment as I was incorrectly thinking that there could be multiple elements assigned to a single strategy instance]

Disregard the part about the race condition - I think I was wrong about that. The comments below stand.