Issue 7084 - fix the compiler's "is recursive" check #8705

KiaraGrouwstra · 2016-05-18T06:49:35Z

Please check if the PR fulfills these requirements

The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit-message-format

I'm not sure if there's much to add in terms of documentation for this fix. Feel free to correct.

I've written a test (based on the Plunker reproduction in the original issue), but I don't see a spec file for the runtime_compiler anymore. Moreover, I don't think my reproduction really qualifies as a compiler unit test, since it's a bit bigger in scope. So I'm not quite sure where you guys would prefer to have me put it -- I'd be glad to get some feedback on this then add it in as desired.

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

Bugfix

What is the current behavior? (You can also link to an open issue here)

See 7084: the compiler currently judges some mutually recursive component structures as non-recursive, which causes it to await non-resolving promises. As a result, in production it hangs without errors, in tests, createAsync just times out to Jasmine.

What is the new behavior?

I let the compiler check the current component's descendants as well, so as to reclassify the misjudged cases as recursive. This way, it no longer hangs/errors.

Does this PR introduce a breaking change?

No

Other information:

From what I can see, the offline compiler has no similar check. I also haven't found equivalent Dart code.

Note I have not yet re-run the test as asked in the guidelines. Like for some other non-Mac users (environment: Windows 10 + Vagrant Ubuntu VM that break on symlinks), building and running the test suite has proven a major barrier for me. I apologize for this, and hope Travis will do for now.

vicb · 2016-05-20T20:57:47Z

modules/@angular/compiler/src/runtime_compiler.ts

@@ -128,6 +128,11 @@ export class RuntimeCompiler implements ComponentResolver {
      var childViewPipes: CompilePipeMetadata[] =
          this._metadataResolver.getViewPipesMetadata(dep.comp.type.runtime);
      var childIsRecursive = ListWrapper.contains(childCompilingComponentsPath, childCacheKey);
+      ListWrapper.forEachWithIndex(childViewDirectives.map(x => x.type.runtime), descendant => {


forEachWithIndex -> forEach however childViewDirectives.map().forEach(...) should work ?

@vicb: whoops, thanks, fair point.

vicb · 2016-05-20T20:59:18Z

@tbosch could you please have a look at the PR and help @tycho01 to come up with a test ?

(@tycho01 please also squash your commits)

KiaraGrouwstra · 2016-05-21T01:44:40Z

Here is the test I'd made based on the original issue (feedback welcome), though I wasn't very sure what might be the best place for it:

// non-returning tests in recursion: [#7084](https://github.com/angular/angular/issues/7084)

import { Component, forwardRef } from '@angular/core';
import { ComponentFixture, NgMatchers, inject, injectAsync, beforeEachProviders, it, fit, xit, expect, afterEach, beforeEach, } from '@angular/core/testing';
import { TestComponentBuilder } from '@angular/compiler/testing';
import { test_comp, asyncTest } from '../../../test';

@Component({
  selector: 'left',
  template: `L<right *ngIf="false"></right>`,
  directives: [
    forwardRef(() => RightComp),
  ]
})
class LeftComp {}

@Component({
  selector: 'right',
  template: `R<left *ngIf="false"></left>`,
  directives: [
    forwardRef(() => LeftComp),
  ]
})
class RightComp {}

@Component({
  selector: 'app',
  template: `[<left *ngIf="false"></left><right *ngIf="false"></right>]`,
  directives: [
    forwardRef(() => LeftComp),
    forwardRef(() => RightComp),
  ]
})
export class App {}

let cls = test_comp('app', App);

describe('7084', () => {
  let tcb;
  let test = (props, fn) => (done) => asyncTest(tcb, cls)(props, fn)(done);

  beforeEach(inject([TestComponentBuilder], (builder) => {
    tcb = builder;
  }));

  it('should work', test({}, ({ comp, el }) => {
    expect(el).toHaveText('[]');
  }));

});

I guess I'll squash them into one when the test is in as well.

tbosch · 2016-06-09T22:49:38Z

This should go into regression_integration_spec (see https://github.com/angular/angular/blob/master/modules/@angular/core/test/linker/regression_integration_spec.ts).

The test looks good, but please add a bit more description than just the issue number.

googlebot · 2016-06-10T13:07:44Z

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

googlebot · 2016-06-10T13:32:27Z

CLAs look good, thanks!

KiaraGrouwstra · 2016-06-10T13:36:55Z

@tbosch: I added the test, vicb's suggestion, and squashed. CI failed on build though... what could I do about that?

tbosch · 2016-06-13T14:49:21Z

modules/@angular/core/test/linker/regression_integration_spec.ts

+        tcb.createAsync(FakeRecursiveComp)
+            .then((fixture) => {
+              fixture.detectChanges();
+              expect(fixture.nativeElement).toHaveText('[]');


Maybe add a bit more expectations to see that the child components really work.

tbosch · 2016-06-13T14:51:03Z

Sorry, master moved ahead, you need to rebase.
As far as I can tell, your last Travis run was green, right? https://travis-ci.org/angular/angular/builds/136680358

googlebot · 2016-06-21T07:05:12Z

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

KiaraGrouwstra · 2016-06-21T07:44:07Z

Um, I tried merging in anything newer, then rebasing everything but the original commit out again in my issue branch. I don't see that reflected here yet, but I'm not sure if that was as you'd intended it.

tbosch · 2016-06-21T17:26:51Z

Somehow you got a lot of commits into the PR. You should only have one that contains the changes you would like to make...

You can try doing a:

git fetch upstream master
git checkout issue-7084
git rebase upstream/master

googlebot · 2016-06-22T02:03:47Z

CLAs look good, thanks!

vicb · 2016-06-22T02:26:03Z

modules/@angular/compiler/src/runtime_compiler.ts

@@ -116,6 +116,11 @@ export class RuntimeCompiler implements ComponentResolver {
      var childViewPipes: CompilePipeMetadata[] =
          this._metadataResolver.getViewPipesMetadata(dep.comp.type.runtime);
      var childIsRecursive = ListWrapper.contains(childCompilingComponentsPath, childCacheKey);


What about:

var childIsRecursive = childCompilingComponentsPath.indexOf(childCacheKey) > -1 || childViewDirectives.some(dir => childCompilingComponentsPath.indexOf(dir.type.runtime) > -1);

Fix how the compiler checks for recursive components by also considering component descendants. Previously, it only checked if the current component was evaluated previously. This failed in certain cases of mutually recursive components, causing `createAsync` in tests to not resolve. closes [7084](angular#7084)

KiaraGrouwstra · 2016-06-22T06:53:20Z

Thanks for the advice to both.

This fix prevented waiting for child components even if the cycle was only introduced via the `directives` array, i.e. without actually having a cycle. This easily causes issues for applications that have one shared list of directives for all components. This reverts commit 3d5bb23.

tbosch · 2016-06-27T20:06:47Z

Hi, I had to revert this commit, see #9647

This fix prevented waiting for child components even if the cycle was only introduced via the `directives` array, i.e. without actually having a cycle. This easily causes issues for applications that have one shared list of directives for all components. This reverts commit 3d5bb23.

This fix prevented waiting for child components even if the cycle was only introduced via the `directives` array, i.e. without actually having a cycle. This easily causes issues for applications that have one shared list of directives for all components. This reverts commit 3d5bb23. Closes #9647

tbosch · 2016-06-28T00:05:42Z

See #9594 for a better fix.

KiaraGrouwstra · 2016-06-28T02:44:17Z

No problem, I'll have to admit it was hard for me to foresee all potential implications. It does make me curious though, since I hadn't really experienced issues with it yet (from monkey-patching to test).

angular-automatic-lock-bot · 2019-09-08T21:21:54Z

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 May 18, 2016

vicb reviewed May 20, 2016
View reviewed changes

mhevery assigned tbosch May 26, 2016

tbosch added pr_state: LGTM action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Jun 9, 2016

KiaraGrouwstra force-pushed the issue-7084 branch from 6fc7083 to 84fd965 Compare June 10, 2016 13:07

googlebot added cla: no and removed cla: yes labels Jun 10, 2016

KiaraGrouwstra force-pushed the issue-7084 branch from 84fd965 to 936dc00 Compare June 10, 2016 13:32

googlebot added cla: yes and removed cla: no labels Jun 10, 2016

tbosch reviewed Jun 13, 2016
View reviewed changes

googlebot added cla: no and removed cla: yes labels Jun 21, 2016

KiaraGrouwstra force-pushed the issue-7084 branch from 50500ff to d31058f Compare June 21, 2016 07:35

KiaraGrouwstra force-pushed the issue-7084 branch from d31058f to d9b2ec8 Compare June 22, 2016 02:03

googlebot added cla: yes and removed cla: no labels Jun 22, 2016

KiaraGrouwstra force-pushed the issue-7084 branch from d9b2ec8 to 927f6a5 Compare June 22, 2016 02:23

vicb reviewed Jun 22, 2016
View reviewed changes

KiaraGrouwstra force-pushed the issue-7084 branch 6 times, most recently from f832dc9 to 15fb42d Compare June 22, 2016 06:07

vicb merged commit 3d5bb23 into angular:master Jun 22, 2016

vicb removed the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Jun 22, 2016

KiaraGrouwstra deleted the issue-7084 branch June 22, 2016 14:10

KiaraGrouwstra mentioned this pull request Jun 22, 2016

[bug] RC.2 breaks @ViewChild to component thru forwardRef (recursive component structures) #9459

Closed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue 7084 - fix the compiler's "is recursive" check #8705

Issue 7084 - fix the compiler's "is recursive" check #8705

KiaraGrouwstra commented May 18, 2016 •

edited

vicb May 20, 2016

KiaraGrouwstra May 21, 2016

vicb commented May 20, 2016 •

edited

KiaraGrouwstra commented May 21, 2016 •

edited

tbosch commented Jun 9, 2016

googlebot commented Jun 10, 2016

googlebot commented Jun 10, 2016

KiaraGrouwstra commented Jun 10, 2016 •

edited

tbosch Jun 13, 2016

tbosch commented Jun 13, 2016

googlebot commented Jun 21, 2016

KiaraGrouwstra commented Jun 21, 2016

tbosch commented Jun 21, 2016

googlebot commented Jun 22, 2016

vicb Jun 22, 2016 •

edited

KiaraGrouwstra commented Jun 22, 2016

tbosch commented Jun 27, 2016

tbosch commented Jun 28, 2016

KiaraGrouwstra commented Jun 28, 2016

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

Issue 7084 - fix the compiler's "is recursive" check #8705

Issue 7084 - fix the compiler's "is recursive" check #8705

Conversation

KiaraGrouwstra commented May 18, 2016 • edited

vicb May 20, 2016

Choose a reason for hiding this comment

KiaraGrouwstra May 21, 2016

Choose a reason for hiding this comment

vicb commented May 20, 2016 • edited

KiaraGrouwstra commented May 21, 2016 • edited

tbosch commented Jun 9, 2016

googlebot commented Jun 10, 2016

googlebot commented Jun 10, 2016

KiaraGrouwstra commented Jun 10, 2016 • edited

tbosch Jun 13, 2016

Choose a reason for hiding this comment

tbosch commented Jun 13, 2016

googlebot commented Jun 21, 2016

KiaraGrouwstra commented Jun 21, 2016

tbosch commented Jun 21, 2016

googlebot commented Jun 22, 2016

vicb Jun 22, 2016 • edited

Choose a reason for hiding this comment

KiaraGrouwstra commented Jun 22, 2016

tbosch commented Jun 27, 2016

tbosch commented Jun 28, 2016

KiaraGrouwstra commented Jun 28, 2016

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

KiaraGrouwstra commented May 18, 2016 •

edited

vicb commented May 20, 2016 •

edited

KiaraGrouwstra commented May 21, 2016 •

edited

KiaraGrouwstra commented Jun 10, 2016 •

edited

vicb Jun 22, 2016 •

edited