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
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
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
Corporate signers
ℹ️ Googlers: Go here for more info.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
CLAs look good, thanks!
ℹ️ Googlers: Go here for more info.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
|
return this.hasOwnProperty(propName) || this.getAttribute(templateName);
|
|
|
}).map(({propName}): [
|
|
|
string, any
|
|
|
] => [propName, (this as any)[propName] || this.getAttribute(templateName)]);
|
yefim
Sep 11, 2020
•
edited
Loading
Uh oh!
There was an error while loading. Please reload this page.
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.
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.
gkalpak
left a comment
•
edited
Loading
Uh oh!
There was an error while loading. Please reload this page.
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:
-
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.)
-
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.
-
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.
Comment on lines
+176
to
+178
|
|
const div = document.createElement('div');
|
|
|
element.appendChild(div);
|
|
|
this.componentRef = this.componentFactory.create(childInjector, projectableNodes, div);
|
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.
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.
aetzlecx
Sep 7, 2021
•
edited
Loading
Uh oh!
There was an error while loading. Please reload this page.
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.
|
|
}
|
|
|
|
|
|
// Re-apply pre-existing values through the strategy.
|
|
|
preExistingValues.forEach(([propName, value]) => strategy.setInputValue(propName, value));
|
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.
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.
|
|
return this.hasOwnProperty(propName) || this.getAttribute(templateName);
|
|
|
}).map(({propName}): [
|
|
|
string, any
|
|
|
] => [propName, (this as any)[propName] || this.getAttribute(templateName)]);
|
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.
|
@yefim any update on this PR status ?
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
Closing this for now. @yefim Feel free to revisit and re-open if you would like to see this move forward.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
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.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Reviewers
2 more reviewers
Reviewers whose approvals may not affect merge requirements
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assigned
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.
|