Skip to content
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.

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

fix(ivy): skip field inheritance if InheritDefinitionFeature is present on parent def #34244

Closed

Conversation

@AndrewKushnir
Copy link
Contributor

AndrewKushnir commented Dec 5, 2019

The main logic of the InheritDefinitionFeature is to go through the prototype chain of a given Component and merge all Angular-specific information onto that Component def. The problem happens in case there is a Component in a hierarchy that also contains the InheritDefinitionFeature (i.e. it extends some other Component), so it inherits all Angular-specific information from its super class. As a result, the root Component may end up having duplicate information inherited from different Components in hierarchy.

Let's consider the following structure: GrandChild extends Child that extends Base and the Base class has a HostListener. In this scenario GrandChild and Child will have InheritDefinitionFeature included into the features list. The processing will happend in the following order:

  • Child inherits HostListener from the Base class
  • GrandChild inherits HostListener from the Child class
  • since Child has a parent, GrandChild also inherits from the Base class

The result is that the GrandChild def has duplicated host listener, which is not correct.

This commit introduces additional logic that checks whether we came across a def that has InheritDefinitionFeature feature (which means that this def already inherited information from its super classes). If that's the case, we skip further fields-related inheritance logic, but keep going though the prototype chain to look for super classes that contain other features (like NgOnChanges), that we need to invoke for a given Component def.

This PR resolves #33300.

PR Type

What kind of change does this PR introduce?

  • Bugfix

Does this PR introduce a breaking change?

  • Yes
  • No
@JoostK

This comment has been minimized.

Copy link
Member

JoostK commented Dec 6, 2019

Related fix done earlier is #30169, it looks like the if (superHostBindings !== prevHostBindings) guard in inheritHostBindings becomes obsolete with these changes.

Copy link
Member

crisbeto left a comment

LGTM, aside from a few minor nits.

@AndrewKushnir AndrewKushnir force-pushed the AndrewKushnir:host_binding_inheritance branch from 0256d24 to c4f1340 Jan 4, 2020
@AndrewKushnir

This comment has been minimized.

Copy link
Contributor Author

AndrewKushnir commented Jan 4, 2020

Thanks for review @crisbeto and @JoostK. I've applied the necessary changes based on your comments.

@kara
kara approved these changes Jan 7, 2020
Copy link
Contributor

kara left a comment

LGTM

…nt on parent def

The main logic of the `InheritDefinitionFeature` is to go through the prototype chain of a given Component and merge all Angular-specific information onto that Component def. The problem happens in case there is a Component in a hierarchy that also contains the `InheritDefinitionFeature` (i.e. it extends some other Component), so it inherits all Angular-specific information from its super class. As a result, the root Component may end up having duplicate information inherited from different Components in hierarchy.

Let's consider the following structure: `GrandChild` extends `Child` that extends `Base` and the `Base` class has a `HostListener`. In this scenario `GrandChild` and `Child` will have `InheritDefinitionFeature` included into the `features` list. The processing will happend in the following order:

- `Child` inherits `HostListener` from the `Base` class
- `GrandChild` inherits `HostListener` from the `Child` class
- since `Child` has a parent, `GrandChild` also inherits from the `Base` class

The result is that the `GrandChild` def has duplicated host listener, which is not correct.

This commit introduces additional logic that checks whether we came across a def that has `InheritDefinitionFeature` feature (which means that this def already inherited information from its super classes). If that's the case, we skip further fields-related inheritance logic, but keep going though the prototype chain to look for super classes that contain other features (like NgOnChanges), that we need to invoke for a given Component def.
…s present on parent def
…s present on parent def
…s present on parent def
@AndrewKushnir AndrewKushnir force-pushed the AndrewKushnir:host_binding_inheritance branch from 748eb45 to 9f9d2fa Jan 7, 2020
@AndrewKushnir AndrewKushnir removed their assignment Jan 7, 2020
@AndrewKushnir

This comment has been minimized.

Copy link
Contributor Author

AndrewKushnir commented Jan 7, 2020

@alxhub alxhub closed this in effb92d Jan 7, 2020
alxhub added a commit that referenced this pull request Jan 7, 2020
…nt on parent def (#34244)

The main logic of the `InheritDefinitionFeature` is to go through the prototype chain of a given Component and merge all Angular-specific information onto that Component def. The problem happens in case there is a Component in a hierarchy that also contains the `InheritDefinitionFeature` (i.e. it extends some other Component), so it inherits all Angular-specific information from its super class. As a result, the root Component may end up having duplicate information inherited from different Components in hierarchy.

Let's consider the following structure: `GrandChild` extends `Child` that extends `Base` and the `Base` class has a `HostListener`. In this scenario `GrandChild` and `Child` will have `InheritDefinitionFeature` included into the `features` list. The processing will happend in the following order:

- `Child` inherits `HostListener` from the `Base` class
- `GrandChild` inherits `HostListener` from the `Child` class
- since `Child` has a parent, `GrandChild` also inherits from the `Base` class

The result is that the `GrandChild` def has duplicated host listener, which is not correct.

This commit introduces additional logic that checks whether we came across a def that has `InheritDefinitionFeature` feature (which means that this def already inherited information from its super classes). If that's the case, we skip further fields-related inheritance logic, but keep going though the prototype chain to look for super classes that contain other features (like NgOnChanges), that we need to invoke for a given Component def.

PR Close #34244
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.