feat(ivy): implement some of the ViewContainerRef API #23189

marclaval · 2018-04-05T12:36:55Z

This PR implements some of the API of the ViewContainerRef and adds more tests.

In scope:

createEmbeddedView
insert
detach
length
get
indexOf
move

Out of scope:

createComponent
remove & clear (destroy part needs to be implemented)

googlebot · 2018-04-05T12:37:50Z

☹️ Sorry, but only Googlers may change the label cla: yes.

pkozlowski-opensource · 2018-04-05T13:45:59Z

packages/core/src/render3/di.ts

@@ -628,22 +628,29 @@ class ViewContainerRef implements viewEngine_ViewContainerRef {
    return viewRef;
  }
  move(viewRef: viewEngine_ViewRef, currentIndex: number): viewEngine_ViewRef {


I know that you are not changing this part but the currentIndex is probably not the best arg name here. How about newIndex?

currentIndex is the name of the arg in the ViewContainerRef abstract class. It makes some sense to keep it here, but it could be changed.

Changed it btw

pkozlowski-opensource · 2018-04-05T13:50:07Z

packages/core/src/render3/di.ts

+    this.detach(index);
+    // TODO(ml): proper destroy of the ViewRef
+  }
+  detach(index?: number|undefined): viewEngine_ViewRef|null {


it is enough to have index?: number (.? adds 'undefined')

pkozlowski-opensource · 2018-04-05T13:52:21Z

packages/core/test/render3/view_container_ref_spec.ts

+      directiveInstance !.vcref.createEmbeddedView(directiveInstance !.tplRef, {name: s}, index);
+    }
+
+    function createTemplate() {


Could you please add code for a template in a comment of this function? It makes it easier to understand what is going on.

pkozlowski-opensource · 2018-04-05T13:55:20Z

packages/core/test/render3/view_container_ref_spec.ts

+      elementEnd();
+    }
+
+    function updateTemplate() {


nit: isn't it too fragile as an utility function with the indexes hard-coded? Maybe you should pass container / element index as arguments to this function?

Not needed yet, let's keep it simple until it is.

pkozlowski-opensource · 2018-04-05T13:57:25Z

packages/core/test/render3/view_container_ref_spec.ts

+    }
+
+    describe('createEmbeddedView (incl. insert)', () => {
+      it('should work on elements', () => {


How is this test different as compared to the existing one here:

angular/packages/core/test/render3/view_container_ref_spec.ts

Line 61 in 5db9ab1

it('should add embedded view into a view container on elements', () => {

Seems like both tests are trying to assert the same thing? Maybe we should merge it into one instead of having 2 similar ones?

I've removed the old ones as the new ones are testing more cases with more concise code (except remove and clear, but it will be done later in another PR).

pkozlowski-opensource · 2018-04-05T13:59:47Z

packages/core/test/render3/view_container_ref_spec.ts

+        expect(() => { createView('Z', 5); }).toThrow();
+      });
+
+      it('should work on containers', () => {


As above we've got a similar test already:

angular/packages/core/test/render3/view_container_ref_spec.ts

Line 130 in 5db9ab1

it('should add embedded view into a view container on ng-template', () => {

Moreover I believe that we can't have a LContainerNode that is both:

created from a JS block

having a directive on it
as it would require sth like:

% if (exp) { //how to add a directive here??? ... % }

See my comment above about refactoring.

About this test, it is true that this precise set of instructions cannot be generated with the current syntax. But it is a unit test which ensures that the embedded views are properly created after the container. So it is valid IMO.

pkozlowski-opensource · 2018-04-05T14:00:53Z

packages/core/test/render3/view_container_ref_spec.ts

+    });
+
+    describe('move', () => {
+      it('should work', () => {


could you think of a more descriptive name for this test?

pkozlowski-opensource · 2018-04-05T14:03:13Z

packages/core/test/render3/view_container_ref_spec.ts

+    });
+
+    describe('get and indexOf', () => {
+      it('should work', () => {


could you think of a more descriptive name for this test?

pkozlowski-opensource · 2018-04-05T14:04:34Z

packages/core/test/render3/view_container_ref_spec.ts

+        createView('B');
+        createView('C');
+        fixture.update();
+        fixture.hostElement.childNodes[1].nodeValue = '**A**';


Could we have, somewhere in a comment, how the template for this test looks like? I find it hard to see what this tests is asserting and if things are working correctly...

Maybe also add a comment explaining why we are doing DOM manipulation here?

Added a comment and more expect to show all the steps of the test.

pkozlowski-opensource · 2018-04-05T14:08:01Z

@marclaval I did the first pass on this PR, see my comments in the code. Looks good, generally speaking, IMO tests need more work (let's make sure that we are not duplicating those, add more comments etc.).

Also you will have to run gulp format to fix lint errors.

Since we are both touching the same area it would be great to have your opinion on #23193

marclaval · 2018-04-05T16:38:17Z

All comments addressed, thanks for the review

mhevery · 2018-04-05T16:45:40Z

packages/core/src/render3/di.ts

  remove(index?: number): void {
-    const adjustedIdx = this._adjustAndAssertIndex(index);
+    this.detach(index);
+    // TODO(ml): proper destroy of the ViewRef


could you add to your comments what steps are missing from here. Otherwise this TODO becomes confusing in the future.

mhevery · 2018-04-05T16:46:10Z

packages/core/src/render3/di.ts

  }
-  indexOf(viewRef: viewEngine_ViewRef): number { throw notImplemented(); }
+  indexOf(viewRef: viewEngine_ViewRef): number { return this._viewRefs.indexOf(viewRef); }


nit: Can you leave empty line between methods.

mhevery · 2018-04-05T16:48:39Z

packages/core/test/render3/view_container_ref_spec.ts


 describe('ViewContainerRef', () => {
-  class TestDirective {
-    constructor(public viewContainer: ViewContainerRef, public template: TemplateRef<any>, ) {}
+  describe('API', () => {


what is the value of this additional describe?

The goal is to group pure API tests, before adding more describe in future PRs, like projection, query, lifecycle hooks

mhevery · 2018-04-05T17:03:54Z

Please add merge label once you take care of the nits

vicb · 2018-04-05T17:17:37Z

packages/core/src/render3/di.ts


-  private _adjustAndAssertIndex(index?: number|undefined) {
+  private _adjustAndAssertIndex(index?: number|undefined, shift: number = 0) {


Should assert be in the name (it's a debug thing) ?

ngbot · 2018-04-05T17:18:51Z

Hi @marclaval! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

vicb · 2018-04-05T17:19:15Z

packages/core/src/render3/di.ts

      // +1 because it's legal to insert at the end.
-      ngDevMode && assertLessThan(index, this._lContainerNode.data.views.length + 1, 'index');
+      ngDevMode &&
+          assertLessThan(index, this._lContainerNode.data.views.length + 1 + shift, 'index');


refactor to have

if (ngDevMode && index !==null) { // asserts return index; } return this._lContainerNode.data.views.length + shift;

This wouldn't work when ngDevMode is false

Well yep there is an error in my extract but the point here is

multiple ngDevMode && does not help with readability,

especially when the else branch is empty in non dev mode

So maybe

if (index === null) return ... if (ngDevMode) { // assert } return ...;

vicb · 2018-04-05T17:20:41Z

packages/core/test/render3/render_util.ts

-  constructor(private createBlock: () => void, private updateBlock: () => void = noop) {
+  constructor(
+      private createBlock: () => void, private updateBlock: () => void = noop,
+      directives?: DirectiveTypesOrFactory|null, pipes?: PipeTypesOrFactory|null) {
    super();
    this.updateBlock = updateBlock || function() {};


needed ? (defalut is noop)

mary-poppins · 2018-04-06T08:32:33Z

You can preview 157a25e at https://pr23189-157a25e.ngbuilds.io/.
You can preview 85c1c50 at https://pr23189-85c1c50.ngbuilds.io/.

mary-poppins · 2018-04-06T08:32:34Z

You can preview 944452f at https://pr23189-944452f.ngbuilds.io/.

marclaval · 2018-04-06T12:35:42Z

Nits integrated and PR rebased.

One job is failing in Travis because of a bundle size issue, but the same is happening on master.
So I've added the merge label as suggested by @mhevery

trotyl · 2018-04-06T12:52:14Z

packages/core/src/render3/assert.ts

@@ -40,6 +40,12 @@ export function assertLessThan<T>(actual: T, expected: T, msg: string) {
  }
 }

+export function assertGreaterThan<T>(actual: T, expected: T, msg: string) {


Generic really needed? Any possibility other than number for <= operator?

@trotyl Could be used for simple alphabetical sorting too, couldn't it? All the other assert functions are implemented the same way, so I suppose this is for consistency above all else.

vicb

see inline comments

mary-poppins · 2018-04-06T20:45:30Z

You can preview a8619b3 at https://pr23189-a8619b3.ngbuilds.io/.

marclaval · 2018-04-06T21:00:58Z

@vicb I did the change you requested, the PR should be good to go

vicb · 2018-04-07T01:23:40Z

http://test/OCL:191967914:BASE:191967926:1523064200979:db19cb5

angular-automatic-lock-bot · 2019-09-13T12:50:59Z

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

googlebot added the cla: yes label Apr 5, 2018

marclaval added action: review The PR is still awaiting reviews from at least one requested reviewer comp: ivy and removed cla: yes labels Apr 5, 2018

googlebot added the cla: yes label Apr 5, 2018

marclaval added the target: major This PR is targeted for the next major release label Apr 5, 2018

pkozlowski-opensource reviewed Apr 5, 2018

View reviewed changes

marclaval force-pushed the vcref_api branch from 157a25e to 85c1c50 Compare April 5, 2018 16:25

marclaval mentioned this pull request Apr 5, 2018

Vcref finding rnode #23193

Closed

mhevery approved these changes Apr 5, 2018

View reviewed changes

mhevery removed the action: review The PR is still awaiting reviews from at least one requested reviewer label Apr 5, 2018

vicb reviewed Apr 5, 2018

View reviewed changes

marclaval force-pushed the vcref_api branch from 85c1c50 to 944452f Compare April 6, 2018 08:23

marclaval added the action: merge The PR is ready for merge by the caretaker label Apr 6, 2018

trotyl reviewed Apr 6, 2018

View reviewed changes

vicb suggested changes Apr 6, 2018

View reviewed changes

marclaval force-pushed the vcref_api branch 2 times, most recently from 5299f3c to a8619b3 Compare April 6, 2018 20:34

feat(ivy): implement some of the ViewContainerRef API

a8619b3

vicb approved these changes Apr 7, 2018

View reviewed changes

marclaval mentioned this pull request Apr 9, 2018

Vcref projection #23272

Closed

IgorMinar closed this in f4c56f4 Apr 9, 2018

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 13, 2019


		private _adjustAndAssertIndex(index?: number\|undefined) {
		private _adjustAndAssertIndex(index?: number\|undefined, shift: number = 0) {

feat(ivy): implement some of the ViewContainerRef API #23189

feat(ivy): implement some of the ViewContainerRef API #23189

Conversation

marclaval commented Apr 5, 2018

googlebot commented Apr 5, 2018

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pkozlowski-opensource commented Apr 5, 2018

marclaval commented Apr 5, 2018

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

mhevery commented Apr 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ngbot bot commented Apr 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vicb Apr 6, 2018 • edited

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

mary-poppins commented Apr 6, 2018

mary-poppins commented Apr 6, 2018

marclaval commented Apr 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vicb left a comment

Choose a reason for hiding this comment

mary-poppins commented Apr 6, 2018

marclaval commented Apr 6, 2018

vicb commented Apr 7, 2018

angular-automatic-lock-bot bot commented Sep 13, 2019

vicb Apr 6, 2018 •

edited