Skip to content

fix(ivy): Implement remaining methods for DebugNode#27387

Closed
mhevery wants to merge 6 commits intoangular:masterfrom
mhevery:testbed_core
Closed

fix(ivy): Implement remaining methods for DebugNode#27387
mhevery wants to merge 6 commits intoangular:masterfrom
mhevery:testbed_core

Conversation

@mhevery
Copy link
Contributor

@mhevery mhevery commented Dec 1, 2018

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.io application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@mary-poppins
Copy link

You can preview bc71ec8 at https://pr27387-bc71ec8.ngbuilds.io/.

@mary-poppins
Copy link

You can preview d587162 at https://pr27387-d587162.ngbuilds.io/.

@mary-poppins
Copy link

You can preview a42c21c at https://pr27387-a42c21c.ngbuilds.io/.

Copy link
Contributor

Choose a reason for hiding this comment

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

In ViewEngine it is possible to workaround Host restriction by providing viewProvider on host element.
It's widely used in forms(https://www.youtube.com/watch?v=CD_t3m2WMM8&feature=youtu.be&t=2051)

See Stackblitz for example: https://stackblitz.com/edit/angular-inncsy?file=src%2Fapp%2Fapp.component.ts

It was achieved by this code

this.viewContext.viewProviders.get(tokenReference(dep.token !)) != null) {
and we also didn't stop traversing the node injector in resolution algorithm since host restriction was on a parser side
while (searchView) {

In current implementation of NodeInjector it seems won't work since we leave traversing as soon as we meet host decorator in https://github.com/angular/angular/blob/a42c21c666914df419fb92d77e434ed8ce5656c0/packages/core/src/render3/di.ts#L547

Ivy example can be found here https://alexzuza.github.io/ivy-jit-preview/#hVNNb9swDD1Pv4LoxTHgLPc0K9IvbDusK9BjEaCqzaRCLUugZbdB4J_VH7F_NUqOXacx2ksiUXyPj4.00taQgx1cGj4VWLgErhRh6lSNCfwubMWRf29a0jPH3RZkCf0lgZvNH5NVOade1wy.1so5pAT.Vi4gf5mSf..elb3DfA0NrMloiJay2FS5pFlqCKNToToZNpdubUhfkHkpka62hdQqPcZ1edPHNnGatZlDrj1HK_BrCoaKZW_DZCcASszZCUNziPR2Kq2NEo461B6Lc3jgG8AiUzVsyFT2LNy_LdInlWdni1n73ybNOMsfH0QTC3wNGtNcliWcW9uXhV0jWEY_go8y7kOdVXRE8tPHe9iXNKkpHJn8RmocIbt8fx1QMgXDSkeVZ5ks_WwnMSy76fLZSuIm5h_UxDyMRjSf2RucGndX.R2EgWBwW4s_Thy.upPgqIfVCl9uydQqQyrncB.whzI4tDpu1Rce2C.Czm6tg8x2oTzpwUatfNkMmYakU2xMV3U4z2RERxsb83j_ciDJq_bxR2Mcey8tlxlWOG6JX_c777dg_IuaxN97wn2rPSw.Ff8B

@mhevery @pkozlowski-opensource @kara @JoostK
Is it by design to change this behavior?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See Stackblitz for example: https://stackblitz.com/edit/angular-inncsy?file=src%2Fapp%2Fapp.component.ts

@alexzuza Thanks for the stackblitz, but when I click on it I have no idea what to do or where to look. Please try to imagine it from my point of view. Instruction which would take me through such links would save me tons of time, instead of reverse engineering it.

Looking at your example I think it should still work with this change. I don't see a breaking change. Also it is not clear from your example which GroupDirective are you expecting? One which is directive or the one declared in viewProviders?

Ivy example can be found here https://alexzuza.github.io/ivy-jit-preview/#hVNNb9swDD1Pv4LoxTHgLPc0K9IvbDusK9BjEaCqzaRCLUugZbdB4J_VH7F_NUqOXacx2ksiUXyPj4.00taQgx1cGj4VWLgErhRh6lSNCfwubMWRf29a0jPH3RZkCf0lgZvNH5NVOade1wy.1so5pAT.Vi4gf5mSf..elb3DfA0NrMloiJay2FS5pFlqCKNToToZNpdubUhfkHkpka62hdQqPcZ1edPHNnGatZlDrj1HK_BrCoaKZW_DZCcASszZCUNziPR2Kq2NEo461B6Lc3jgG8AiUzVsyFT2LNy_LdInlWdni1n73ybNOMsfH0QTC3wNGtNcliWcW9uXhV0jWEY_go8y7kOdVXRE8tPHe9iXNKkpHJn8RmocIbt8fx1QMgXDSkeVZ5ks_WwnMSy76fLZSuIm5h_UxDyMRjSf2RucGndX.R2EgWBwW4s_Thy.upPgqIfVCl9uydQqQyrncB.whzI4tDpu1Rce2C.Czm6tg8x2oTzpwUatfNkMmYakU2xMV3U4z2RERxsb83j_ciDJq_bxR2Mcey8tlxlWOG6JX_c777dg_IuaxN97wn2rPSw.Ff8B

WOW that looks amazing. Except I don't know what it is trying to tell me. Some narrative would be great. Without an explanation of what I am looking at it is just a pretty picture.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at your example I think it should still work with this change. I don't see a breaking change. Also it is not clear from your example which GroupDirective are you expecting? One which is directive or the one declared in viewProviders?

@mhevery
Sorry for unclear demonstration.


ViewEngine

I changed the demo. https://stackblitz.com/edit/angular-riss8k?file=src/app/app.component.ts

I'm expecting GroupDirective (which is on <div group>) to be injected in ControlNameDirective.

  1. If you'll open the demo you will see the error

Template parse errors:
No provider for ControlContainer ("
[ERROR ->]
"): ng:///AppModule/ChildComponent.html@1:4

  1. Once you uncomment the line 38
// { provide: ControlContainer, useExisting: GroupDirective }

the error goes away.

That's the case how we can workaround nested forms. (It's shown on @kara's presentation https://www.youtube.com/watch?v=CD_t3m2WMM8&feature=youtu.be&t=2051)


Ivy

Now, if you'll open the following ivy demo with the same workaround
https://alexzuza.github.io/ivy-jit-preview/#tVPLbtswEDyXX7HIRRIg23f5ATeO0RRo0gLuLTAQRqIdIpLIrijHhqHP6kf0r7qkLFWOjOTUi0Qud2ZnOUuZaYUGjrBQtMpFbkK4kShiI3cihK.5Liny53fG8YXi5gC8gHYTwv32TiVlSqnLHYGXmTRGYAjfS.OQt6qg7.pF6pVIN1DBBlUG3pzn2zLlOIoVCm_MZCNDp9xsFGbXqF4LgTeHnGcy7uOavMFTnThI6swu14mjFvgxBUHZvL0G_8gACpHSTSiMwMsOA661F1LUiMxiRQSPtAOYJHIHW1Slnrn9p0n8LNNkNhnV_zppRFl2.ciqgIm90xinvCjgs9ZtWThWjLE6vlC5QZXaH5e5QHc2b915q_DBSVg7iRrVTiYCiwgeXPljE4p6tCGUhVjuZWFkvo3gi2Vpi0BF8HVP8pskJ_odZXFd8p5nYu31yBb_TjuUREGwwmBpWfy5nSQ_gHkzS7TWHOnK.h0F1G7FqvfsdM5cdlPamYeOZDAHLaZXRuzNlXPQwnZSvP74z7e8sCI7o8FcT82Tcy3Vw24FnE372kpMBNEgN5KusVHYnbXQRc6V1LFLjpxOziRZ1Tb.pJQhp7imMt0K_Zbo9PQe7cxcfu1.MGwJT622sGAYcxM_.75Asnk6c3OSqLjMqNzwVynwsDq57DdvNhjKnBy4_Xn3DaZAwDHJGrO_
(on demo I use almost last code from angular master branch)

and click on Start button

1

You will get the error below

Error: NodeInjector: NOT_FOUND [ControlContainer]

Copy link
Member

@JoostK JoostK Dec 3, 2018

Choose a reason for hiding this comment

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

@alexzuza that looks like a bug in ViewEngine. ViewEngine only performs @Host checking during template compiles, but I'm not sure it takes directives into account. In Ivy, the @Host is checked at runtime in the node injector itself during resolving.

In your example, the ControlNameDirective is applied to the <input controlName type="text"> element. That particular element is the directive's host, not the component itself. So, the viewProvider of the container is not actually in scope, hence Ivy throws.

I discovered one more scenario where ViewEngine is able to resolve an @Host decorated param where Ivy is not, namely when dynamically creating a component. My understanding has always been that the element from which the component is created dynamically acts as the host, but Ivy has proven that not to be true. And that makes sense, because even for dynamically created components you'll get an actual host element according to the created component's selector (or ng-component if it has no selector).

In ViewEngine, the @Host decorator is verified in the parent template where the host element originates from. For dynamically created components no such static host element exists so ViewEngine's template parser does not enforce in any way the @Host decorator. ViewEngine would happily traverse the full node injector tree to resolve an @Host token, which is not desirable.

So, in conclusion: yes, you may find that Ivy is unable to resolve @Host annotated providers where ViewEngine was able to, but that actually boils down to a limitation in ViewEngine's checks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alexzuza Let me get your test case into a spec file and see if that works as expected. I will do that in a separate PR as this one is getting large and I want to land it.

Copy link
Contributor

Choose a reason for hiding this comment

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

@JoostK Thank you so much for taking the time sharing your thoughs on this. The Host decorator always confuses me in such scenarios.

@mhevery Thank you for looking at this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@JoostK thanks for your comment. I still don't know what the correct behavior is.

In your example, the ControlNameDirective is applied to the <input controlName type="text"> element. That particular element is the directive's host, not the component itself. So, the viewProvider of the container is not actually in scope, hence Ivy throws.

This makes a lot of sense to me, so I would agree with you that this is just a bug in ViewEngine and we should not be replicating the bug in Ivy.

So, in conclusion: yes, you may find that Ivy is unable to resolve @Host annotated providers where ViewEngine was able to, but that actually boils down to a limitation in ViewEngine's checks.

So it sounds to me as if Ivy is working as intended. and we should close #27444

/cc @marclaval

@mary-poppins
Copy link

You can preview 50e877b at https://pr27387-50e877b.ngbuilds.io/.

Copy link
Member

@pkozlowski-opensource pkozlowski-opensource left a comment

Choose a reason for hiding this comment

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

Thnx @mhevery for addressing my comments, it LGTM now.

@mary-poppins
Copy link

You can preview 301e775 at https://pr27387-301e775.ngbuilds.io/.

@kara kara added action: review The PR is still awaiting reviews from at least one requested reviewer target: major This PR is targeted for the next major release comp: ivy labels Dec 3, 2018
@ngbot ngbot bot added this to the needsTriage milestone Dec 3, 2018
Copy link
Contributor

@kara kara left a comment

Choose a reason for hiding this comment

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

LGTM, aside from symbol test failures

@mary-poppins
Copy link

You can preview b7dbd02 at https://pr27387-b7dbd02.ngbuilds.io/.

@mhevery
Copy link
Contributor Author

mhevery commented Dec 3, 2018

@mary-poppins
Copy link

You can preview 959628f at https://pr27387-959628f.ngbuilds.io/.

@mhevery mhevery added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Dec 3, 2018
@mary-poppins
Copy link

You can preview 5deae24 at https://pr27387-5deae24.ngbuilds.io/.

@mary-poppins
Copy link

You can preview cbd5d0e at https://pr27387-cbd5d0e.ngbuilds.io/.

@IgorMinar IgorMinar closed this in b2d6f43 Dec 5, 2018
cexbrayat added a commit to cexbrayat/angular that referenced this pull request Feb 9, 2019
It has not been used since angular#27387 implemented the last missing methods in DebugNode
@angular-automatic-lock-bot
Copy link

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.

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

Labels

action: merge The PR is ready for merge by the caretaker cla: yes target: major This PR is targeted for the next major release type: bug/fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants