GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Allow Angular Element to be removed and readded to the DOM with all inputs and outputs initialized and hooked up by yefim · Pull Request #38816 · angular/angular

Conversation

@yefim

@yefim yefim commented Sep 11, 2020

Copy link
Copy Markdown

Fixes #38778

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
yefim added 2 commits September 11, 2020 12:29
@yefim
Set inputs on every connect
609df7c
@yefim
Update strategy to allow for removing and readding elements
820a091
@googlebot

googlebot commented Sep 11, 2020

Copy link
Copy Markdown

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
  • It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.
Corporate signers
  • Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
  • The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
  • The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@googlebot googlebot added the cla: no label Sep 11, 2020
@pullapprove
pullapprove Bot requested a review from gkalpak September 11, 2020 19:33
@sonukapoor sonukapoor added the area: elements Issues related to Angular Elements label Sep 11, 2020
@ngbot ngbot Bot added this to the needsTriage milestone Sep 11, 2020
@googlebot

googlebot commented Sep 11, 2020

Copy link
Copy Markdown

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@googlebot googlebot added cla: yes and removed cla: no labels Sep 11, 2020
@yefim
Grab the DOM attribute
4d0373e
yefim
yefim commented Sep 11, 2020
View reviewed changes
Comment thread packages/elements/src/create-custom-element.ts
return this.hasOwnProperty(propName) || this.getAttribute(templateName);
}).map(({propName}): [
string, any
] => [propName, (this as any)[propName] || this.getAttribute(templateName)]);

@yefim yefim Sep 11, 2020 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will override any falsey HTML properties (false, 0, ''). A solution I thought of would be to reduce over the inputs instead of filter then map. However, .reduce() is less readable.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@gkalpak gkalpak Sep 13, 2020

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't thought this through, but I 99% sure there are edgecases that will break with this approach 😁
In any case, we should make sure that falsy property values are not overriden by attribute values, but I confident even that won't make this 100% safe 😅

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
yefim added 2 commits September 11, 2020 12:53
@yefim
Use hasAttribute instead of getAttribute
69b136b
@yefim
Remove unneeded assignment
4165efb
gkalpak
gkalpak suggested changes Sep 13, 2020
View reviewed changes

@gkalpak gkalpak left a comment •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thx for putting together a pull request, @yefim!

I am still wary of getting down the path of trying to re-create the component instance, because there are too many concerns/edge cases to account for, including:

  1. How do we recreate the destroyed component's state?

    For regular Angular components (not Angular elements), this is easy because the component's inputs come through templates and other components' properties. For Angular elements however, people might in addition have interacted with the host element directly (both via setting attributes and properties).

    For example, it is hard to know what value to initialize an input to. Should we use the value from the host element attribute, the host element property or the previous component instance's property? (The component could be updating these values internally without reflecting the changes on the host element attributes/properties. Or the host element properties could have been updated with new values after the component has been destroyed.)

  2. Even if we managed to re-create the component's external state, its internal/UI state could be different. This might be expected when explicitly re-creating a component in an Angular all (e.g. via a structural directive such as ngIf). However, in an Angular element setup, people interacting with a custom element would expect it to behave like most regular HTML elements, which don't lose their internal state when detached from the DOM for a brief time.

  3. How do we handle projected nodes?

    Do we clone the originally projected nodes and re-insert them into the host element? This would break any external references to the nodes?

    Do we keep references to the originally projected nodes (and their assigned projection slots) and use those when instantiating the new component? What if the nodes have been replaced in the meantime (e.g. by a structural directive)?

I am not saying this is impossible to get right, but it is definitely an uphill battle (and a maintenance burden). But I would rather explore other approaches first.

Basically, the problem we are trying to solve is this: Keep a removed component intact when it is going to be used again in the future, but allow it to be garbage-collected when not needed any more.

The difficulty arises from the fact that Angular itself does not know what the intention is when an Angular element is removed from the DOM. Possible solutions (off the top of my head):

  • Have a way to "suspend" a component (i.e. detach it from the app's internal processing, such as change detection), but otherwise keep its internal state. But do this in a way that would allow the component (and associated data) to be garbage-collected if its host element is not referenced any more. This way, the component would remain intact as long as we have a reference to its host element (because we intend to add it back to the DOM later), but could be garbage-collected if we don't hold onto the host element.

    One caveat is that when unsuspending the component it's inputs should be populated with the latest values.

  • Have an (opt-in) way to give the app developer the power (and responsibility) to decide if/when a component will be destroyed (see #38778 (comment) for how to hack your way to this today). This way, component's would be destroyed by default when removed from the DOM, but the user could choose to keep them around and manually trigger their destruction when needed.

    For example, there could be a method on the element to skip destroying the component. Or there could be a new NgElementStrategy that does not destroy components when removed from the DOM. In both cases, there would be a method to call when you want to destroy an Angular element component.

@yefim, wdyt?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread packages/elements/src/component-factory-strategy.ts
Comment on lines +176 to +178
const div = document.createElement('div');
element.appendChild(div);
this.componentRef = this.componentFactory.create(childInjector, projectableNodes, div);

@gkalpak gkalpak Sep 13, 2020

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue this is trying to address is valid (see #38778 (comment)), but I don't think this is the right solution.

I would rather fix it in @angular/animations (or at least in some way address the animation issue in @angular/elements). Could you open an separate issue to track that?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@yefim yefim Sep 23, 2020

Copy link
Copy Markdown
Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've created the bug against @angular/animations. #38962

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@aetzlecx aetzlecx Sep 7, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gkalpak / @yefim I applied the fix code from this pull request as a patch into our customer's workspace and I can confirm that simple use cases are working.

As you stated in your last comment in 1) and 2) I can totally confirm that the fix is not working for complex components e.g. components that have complex attributes (non string) or components that have a state maintained or have a changed state. Unfortunately I cannot provide more details due to client restrictions.

So sadly I have to confirm that this fix will not work, at least for our solution I cannot confirm that it brings improvements.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread packages/elements/src/create-custom-element.ts
}

// Re-apply pre-existing values through the strategy.
preExistingValues.forEach(([propName, value]) => strategy.setInputValue(propName, value));

@gkalpak gkalpak Sep 13, 2020

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am afraid we can't remove this bit of logic from here. It is not guaranteed that connectedCallback() will be called before trying to access the strategy (as is shown by the failing tests on CI).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@yefim yefim Sep 23, 2020

Copy link
Copy Markdown
Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. I can add it back if we decide to pursue my solution.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread packages/elements/src/create-custom-element.ts
return this.hasOwnProperty(propName) || this.getAttribute(templateName);
}).map(({propName}): [
string, any
] => [propName, (this as any)[propName] || this.getAttribute(templateName)]);

@gkalpak gkalpak Sep 13, 2020

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't thought this through, but I 99% sure there are edgecases that will break with this approach 😁
In any case, we should make sure that falsy property values are not overriden by attribute values, but I confident even that won't make this 100% safe 😅

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@gkalpak gkalpak mentioned this pull request Sep 13, 2020
Angular Element is not removed and readded to the DOM properly #38778
Open
@yefim yefim mentioned this pull request Sep 23, 2020
Angular Element DOM node is removed from DOM when an Angular Element is readded/reconnected #38962
Closed
@iRealNirmal

iRealNirmal commented May 18, 2021

Copy link
Copy Markdown
Contributor

@yefim any update on this PR status ?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@jessicajaniuk

jessicajaniuk commented Feb 4, 2022

Copy link
Copy Markdown
Contributor

Closing this for now. @yefim Feel free to revisit and re-open if you would like to see this move forward.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@jessicajaniuk jessicajaniuk closed this Feb 4, 2022
@angular-automatic-lock-bot

angular-automatic-lock-bot Bot commented Mar 7, 2022

Copy link
Copy Markdown

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@angular-automatic-lock-bot angular-automatic-lock-bot Bot locked and limited conversation to collaborators Mar 7, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers
@aetzlecx aetzlecx aetzlecx left review comments
@gkalpak gkalpak gkalpak requested changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

area: elements Issues related to Angular Elements cla: yes

Projects

None yet

Milestone

needsTriage

Development

Successfully merging this pull request may close these issues.

Angular Element is not removed and readded to the DOM properly

Uh oh!

There was an error while loading. Please reload this page.

7 participants

@yefim @googlebot @iRealNirmal @jessicajaniuk @gkalpak @aetzlecx @sonukapoor
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.