fix(ivy): `ViewRef.rootNodes` returns rootNodes from child views as well #33457

mhevery · 2019-10-28T23:58:37Z

The idea behind rootNodes is to return all of the nodes which need
to be added to the DOM in order for the ViewRef to be rendered.
The new implementation correctly takes into account the case where
one of the rootNodes is a container containing other ViewRefs.

…ell. The idea behind `rootNodes` is to return all of the nodes which need to be added to the DOM in order for the `ViewRef` to be rendered. The new implementation correctly takes into account the case where one of the rootNodes is a container containing other `ViewRef`s.

…ws as well.

packages/core/src/render3/view_ref.ts

pkozlowski-opensource · 2019-10-29T11:14:22Z

packages/core/src/render3/view_ref.ts

-    if (tNodeChild.type === TNodeType.ElementContainer) {
-      collectNativeNodes(lView, tNodeChild, result);
-    } else if (tNodeChild.type === TNodeType.Projection) {
+    if (tNode.type === TNodeType.ElementContainer) {


I don't see a test for the TNodeType.IcuContainer in the series of node type checks. If I recall properly ICU containers "behave" similarly to <ng-container>s. I'm not sure if we can bump into ICU containers here, but:

if no, we should add an assert;

if yes, we should add a test + handle this case.

Also, I would add an assert to verify that we don't bump into tNode being TNodeType.View (just for completeness, to make sure that we are not missing handling of any TNode type).

I have added asserts

Added tests and was not able to get Icu node to show up here.

packages/core/src/render3/view_ref.ts

pkozlowski-opensource · 2019-10-29T15:36:26Z

packages/core/test/acceptance/view_container_ref_spec.ts

+        selector: 'edit-form',
+        template: `
+          <ng-template [dir]="true">
+            <div *ngIf="true">Text</div>


Could we add few more test cases:

nested <ng-container>s at the root of an embedded view;

ICU inside <ng-container> - I'm not sure if we can have them at the root of a view;

a directive that asks for a ViewContainerRef at the root of a view (you've got a case where a VCRef is asked on a container);

added test cases

I haven't seen a test for the case where a ViewContainerRef is injected on a regular element.

pkozlowski-opensource

Did the first pass. The direction LGTM but I'm not sure if we are handling all the cases of nodes that can be at the root of a view (left comments enumerating those cases). As the very minimum we need more tests.

…ws as well.

mhevery

Updated, PTAL

mhevery · 2019-10-29T22:24:21Z

packages/core/src/render3/view_ref.ts

-    if (tNodeChild.type === TNodeType.ElementContainer) {
-      collectNativeNodes(lView, tNodeChild, result);
-    } else if (tNodeChild.type === TNodeType.Projection) {
+    if (tNode.type === TNodeType.ElementContainer) {


I have added asserts

Added tests and was not able to get Icu node to show up here.

mhevery · 2019-10-29T22:24:34Z

packages/core/test/acceptance/view_container_ref_spec.ts

+        selector: 'edit-form',
+        template: `
+          <ng-template [dir]="true">
+            <div *ngIf="true">Text</div>


added test cases

…ws as well.

pkozlowski-opensource · 2019-10-30T10:55:30Z

packages/core/src/render3/view_ref.ts

+    } else if (tNode.type === TNodeType.Container) {
+      const lContainer = lView[tNode.index];
+      const containerTView = tNode.tViews !as TView;
+      ngDevMode && assertDefined(containerTView, 'TView expected');


I don't think that this assert is correct for 2 reasons:

it refers to a TView defined by this <ng-template> and not inserted into a ViewContainerRef asked on this <ng-template>;

many different types of TViews can be inserted into one container so we can't assume that all inserted views share the same TView

it can be null when the definition is empty <ng-template></ng-template>, AFAIK

There is a confusion between an embedded view defintion and the the inserted instance here.

pkozlowski-opensource · 2019-10-30T13:02:25Z

packages/core/test/acceptance/view_container_ref_spec.ts

+          <ng-template [dir]="true">
+            <div *ngIf="true">Text</div>
+            <ng-container>container-content</ng-container>
+            <div i18n>assert TNodeType.IcuContainer is never a root node</div>


This is not enough, you need to have an actual ICU expression inside an element with the i18n attribute. Also, having div at the root is not enough as a <div> is a root node so no need to descend any further. An example way of testing it would be <ng-container i18n>Updated {minutes, plural, =0 {just now} =1 {one minute ago} other {several minutes ago}}</ng-container> and here we need to descent into the ICU container.

tl;dr; I still believe that we need to handle the case of a ICU container.

pkozlowski-opensource · 2019-10-30T13:03:08Z

packages/core/src/render3/view_ref.ts

+  while (tNode) {
+    ngDevMode && assertNodeOfPossibleTypes(
+                     tNode, TNodeType.Element, TNodeType.Container, TNodeType.Projection,
+                     TNodeType.ElementContainer);


I believe that we need to handle a ICU container, see: https://github.com/angular/angular/pull/33457/files#r340598750

pkozlowski-opensource

@mhevery I still believe that there are tests / use-cases missing. How about we review / merge #33493 instead that has all the test I could think of (+ fixes for those)?

mhevery · 2019-10-30T16:04:30Z

Closing in as @pkozlowski-opensource took over #33493

angular-automatic-lock-bot · 2019-11-30T16:09:58Z

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.}

mhevery requested review from pkozlowski-opensource, kara and AndrewKushnir October 28, 2019 23:58

mhevery requested a review from a team as a code owner October 28, 2019 23:58

googlebot added the cla: yes label Oct 28, 2019

mhevery added action: merge The PR is ready for merge by the caretaker target: major This PR is targeted for the next major release action: review The PR is still awaiting reviews from at least one requested reviewer labels Oct 28, 2019

fixup! fix(ivy): ViewRef.rootNodes returns rootNodes from child vie…

dc424ff

…ws as well.

pkozlowski-opensource reviewed Oct 29, 2019

View reviewed changes

packages/core/src/render3/view_ref.ts Outdated Show resolved Hide resolved

pkozlowski-opensource reviewed Oct 29, 2019

View reviewed changes

packages/core/src/render3/view_ref.ts Outdated Show resolved Hide resolved

pkozlowski-opensource reviewed Oct 29, 2019

View reviewed changes

pkozlowski-opensource requested changes Oct 29, 2019

View reviewed changes

mhevery added 2 commits October 29, 2019 10:38

fixup! fix(ivy): ViewRef.rootNodes returns rootNodes from child vie…

6a4f953

…ws as well.

fixup! fix(ivy): ViewRef.rootNodes returns rootNodes from child vie…

7cfeec9

…ws as well.

mhevery commented Oct 29, 2019

View reviewed changes

fixup! fix(ivy): ViewRef.rootNodes returns rootNodes from child vie…

f27a91e

…ws as well.

pkozlowski-opensource reviewed Oct 30, 2019

View reviewed changes

pkozlowski-opensource mentioned this pull request Oct 30, 2019

Fw 1559 - more cases #33493

Closed

pkozlowski-opensource requested changes Oct 30, 2019

View reviewed changes

mhevery closed this Oct 30, 2019

angular-automatic-lock-bot bot locked and limited conversation to collaborators Nov 30, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(ivy): `ViewRef.rootNodes` returns rootNodes from child views as well #33457

fix(ivy): `ViewRef.rootNodes` returns rootNodes from child views as well #33457

mhevery commented Oct 28, 2019

pkozlowski-opensource Oct 29, 2019

pkozlowski-opensource Oct 29, 2019

mhevery Oct 29, 2019

pkozlowski-opensource Oct 29, 2019

mhevery Oct 29, 2019

pkozlowski-opensource Oct 30, 2019

pkozlowski-opensource left a comment

mhevery left a comment

mhevery Oct 29, 2019

mhevery Oct 29, 2019

pkozlowski-opensource Oct 30, 2019

pkozlowski-opensource Oct 30, 2019

pkozlowski-opensource Oct 30, 2019

pkozlowski-opensource left a comment

mhevery commented Oct 30, 2019

angular-automatic-lock-bot bot commented Nov 30, 2019

fix(ivy): ViewRef.rootNodes returns rootNodes from child views as well #33457

fix(ivy): ViewRef.rootNodes returns rootNodes from child views as well #33457

Conversation

mhevery commented Oct 28, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pkozlowski-opensource left a comment

Choose a reason for hiding this comment

mhevery left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pkozlowski-opensource left a comment

Choose a reason for hiding this comment

mhevery commented Oct 30, 2019

angular-automatic-lock-bot bot commented Nov 30, 2019

fix(ivy): `ViewRef.rootNodes` returns rootNodes from child views as well #33457

fix(ivy): `ViewRef.rootNodes` returns rootNodes from child views as well #33457