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

mutual recursion among components entered from multiple sides #7084

Closed
KiaraGrouwstra opened this issue Feb 15, 2016 · 3 comments
Closed

mutual recursion among components entered from multiple sides #7084

KiaraGrouwstra opened this issue Feb 15, 2016 · 3 comments

Comments

@KiaraGrouwstra
Copy link
Contributor

KiaraGrouwstra commented Feb 15, 2016

While the issue I previously submitted potentially related to mutually recursive components, I found another issue where it's much more clear-cut it triggers due to recursion, plunker here. I have:

  • a LeftComp that 'could' include right: L<right *ngIf="false"></right>
  • a RightComp that 'could' include left: R<left *ngIf="false"></left>
  • a main component that 'could' include both: [ <left *ngIf="false"></left> <right *ngIf="false"></right> ]

Expected result: just the brackets [], since the rest should never trigger.

Actual result: if using a Karma test, then TestComponentBuilder's createAsync's promise never resolves, resulting in a Jasmine timeout; similarly, in the plunker, it remains stuck 'loading' the main component without actually throwing an error.

@KiaraGrouwstra
Copy link
Contributor Author

KiaraGrouwstra commented May 11, 2016

To give an update on this, this still crashes in tests as of RC1. I don't see RC available on the CDN for Plunker yet, but the Plunker holds without changes (other than version) for beta 17.

@KiaraGrouwstra
Copy link
Contributor Author

The culprit is this line:

var childIsRecursive = ListWrapper.contains(childCompilingComponentsPath, childCacheKey);

This incorrectly evaluates to false (not recursive) in the case of the above-mentioned components. (More accurately, it passes by left/right twice -- once from app, once from each other. The latter pass for each should evaluate to recursive for things to work.)

Adding the following snippet after it relaxes the recursion check so as to also evaluate to true when the current component directly references a component checked before.

ListWrapper.forEachWithIndex(childViewDirectives.map(x => x.type.runtime), descendant => {
    if (ListWrapper.contains(childCompilingComponentsPath, descendant)) {
        childIsRecursive = true;
    }
});

I'll try to make a PR for this.

KiaraGrouwstra added a commit to KiaraGrouwstra/angular that referenced this issue May 18, 2016
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 added a commit to KiaraGrouwstra/angular that referenced this issue Jun 10, 2016
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 added a commit to KiaraGrouwstra/angular that referenced this issue Jun 10, 2016
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 added a commit to KiaraGrouwstra/angular that referenced this issue Jun 10, 2016
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 added a commit to KiaraGrouwstra/angular that referenced this issue Jun 22, 2016
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 added a commit to KiaraGrouwstra/angular that referenced this issue Jun 22, 2016
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 added a commit to KiaraGrouwstra/angular that referenced this issue Jun 22, 2016
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 added a commit to KiaraGrouwstra/angular that referenced this issue Jun 22, 2016
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 added a commit to KiaraGrouwstra/angular that referenced this issue Jun 22, 2016
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 added a commit to KiaraGrouwstra/angular that referenced this issue Jun 22, 2016
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 added a commit to KiaraGrouwstra/angular that referenced this issue Jun 22, 2016
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 added a commit to KiaraGrouwstra/angular that referenced this issue Jun 22, 2016
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)
vicb pushed a commit that referenced this issue Jun 22, 2016
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](#7084)
tbosch added a commit to tbosch/angular that referenced this issue Jun 28, 2016
Adds new abstraction `Compiler` with methods
`compileComponentAsync` and `compileComponentSync`.
This is in preparation of deprecating `ComponentResolver`.

`compileComponentSync` is able to compile components
synchronously given all components either have an inline
template or they have been compiled before.

Also changes `TestComponentBuilder.createSync` to
take a `Type` and use the new `compileComponentSync` method.

Also supports overriding the component metadata even if
the component has already been compiled.

Also fixes angular#7084 in a better way.

BREAKING CHANGE:
`TestComponentBuilder.createSync` now takes a component type
and throws if not all templates are either inlined
are compiled before via `createAsync`.
tbosch added a commit to tbosch/angular that referenced this issue Jun 28, 2016
Adds new abstraction `Compiler` with methods
`compileComponentAsync` and `compileComponentSync`.
This is in preparation of deprecating `ComponentResolver`.

`compileComponentSync` is able to compile components
synchronously given all components either have an inline
template or they have been compiled before.

Also changes `TestComponentBuilder.createSync` to
take a `Type` and use the new `compileComponentSync` method.

Also supports overriding the component metadata even if
the component has already been compiled.

Also fixes angular#7084 in a better way.

BREAKING CHANGE:
`TestComponentBuilder.createSync` now takes a component type
and throws if not all templates are either inlined
are compiled before via `createAsync`.
tbosch added a commit to tbosch/angular that referenced this issue Jun 28, 2016
Adds new abstraction `Compiler` with methods
`compileComponentAsync` and `compileComponentSync`.
This is in preparation of deprecating `ComponentResolver`.

`compileComponentSync` is able to compile components
synchronously given all components either have an inline
template or they have been compiled before.

Also changes `TestComponentBuilder.createSync` to
take a `Type` and use the new `compileComponentSync` method.

Also supports overriding the component metadata even if
the component has already been compiled.

Also fixes angular#7084 in a better way.

BREAKING CHANGE:
`TestComponentBuilder.createSync` now takes a component type
and throws if not all templates are either inlined
are compiled before via `createAsync`.
tbosch added a commit to tbosch/angular that referenced this issue Jun 28, 2016
Adds new abstraction `Compiler` with methods
`compileComponentAsync` and `compileComponentSync`.
This is in preparation of deprecating `ComponentResolver`.

`compileComponentSync` is able to compile components
synchronously given all components either have an inline
template or they have been compiled before.

Also changes `TestComponentBuilder.createSync` to
take a `Type` and use the new `compileComponentSync` method.

Also supports overriding the component metadata even if
the component has already been compiled.

Also fixes angular#7084 in a better way.

BREAKING CHANGE:
`TestComponentBuilder.createSync` now takes a component type
and throws if not all templates are either inlined
are compiled before via `createAsync`.
tbosch added a commit to tbosch/angular that referenced this issue Jun 28, 2016
Adds new abstraction `Compiler` with methods
`compileComponentAsync` and `compileComponentSync`.
This is in preparation of deprecating `ComponentResolver`.

`compileComponentSync` is able to compile components
synchronously given all components either have an inline
template or they have been compiled before.

Also changes `TestComponentBuilder.createSync` to
take a `Type` and use the new `compileComponentSync` method.

Also supports overriding the component metadata even if
the component has already been compiled.

Also fixes angular#7084 in a better way.

BREAKING CHANGE:
`TestComponentBuilder.createSync` now takes a component type
and throws if not all templates are either inlined
are compiled before via `createAsync`.

Closes angular#9594
@tbosch tbosch closed this as completed in bf598d6 Jun 28, 2016
@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 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant