feat(NgComponentOutlet): add NgComponentOutlet directive #11235

shlomiassaf · 2016-09-01T11:03:16Z

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
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

What kind of change does this PR introduce? (check one with "x")

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Other... Please describe:

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

What is the new behavior?
See #11168

Does this PR introduce a breaking change? (check one with "x")

[ ] Yes
[x] No

If this PR contains a breaking change, please describe the impact and migration path for existing applications: ...

Other information:

This commits adds a new NgComponentOutlet directive that can be
used to dynamically create host views from a supplied component type/class.

Closes #11168
Closes #9599

vicb · 2016-09-01T17:03:04Z

modules/@angular/router/src/router.ts

I think with commit is unrequired if you rebase on master

Yep, someone got me to it :)

vicb · 2016-09-01T17:48:57Z

modules/@angular/common/src/directives/ng_component_outlet.ts

remove template and document the short syntax ?

<ng-container *ngComponentOutlet="..."></ng-container>

There no gain in using the short syntax since the element will never be used.
The only thing that get's rendered is the component.

I chose to enforce templateso this confusion can be avoided.
In the ng-content example the ng-content tag will be removed and the user won't understand why children won't project.

I had a discussion about this with ~~@alexeagle~~ @alxhub , he also supports that.
I believe this should be in NgTemplateOutlet as well.

However, if i'm missing something we can change this.

he also supports that

but IE doesn't :)

I am with @vicb

<ng-container *ngComponentOutlet="..."></ng-container>

is better because the long form than becomes

<ng-container *ngComponentOutlet="componentTypeExpression; injector: injExp; provider: proExp; projectable: proExp"></ng-container>

This means that all @Input need to be prefixed.

actually, giving it a bit more thought this is even better:

<ng-container *ngOutlet="component: componentTypeExpression; injector: injExp; provider: proExp; projectable: proExp"></ng-container>

This commits adds a new NgComponentOutlet directive that can be used to dynamically create host views from a supplied component type/class. Closes angular#11168

shlomiassaf · 2016-09-04T16:16:20Z

@vicb Should I do something regarding the pr_action: cleanup?

The TRIAGE_AND_LABELS.md says:

pr_action: cleanup -- more work is needed from the current assignee.

Who's the 'assignee'?

vicb · 2016-09-04T16:47:32Z

we won't merge new features into master for for next few weeks only bug fixes,
you still have not address my comment for IE not supporting template

shlomiassaf · 2016-09-06T00:33:46Z

@vicb Good point on IE, will fix.

vicb · 2016-09-16T05:04:06Z

Could you please rebase ?

vicb · 2016-09-16T05:03:35Z

modules/@angular/common/src/directives/ng_component_outlet.ts

+@Directive({selector: 'template[ngComponentOutlet]'})
+export class NgComponentOutlet implements OnChanges {
+  @Input() ngComponentOutlet: Type<any>;
+  @Input() ngOutletInjector: Injector;


all input must start with the ngComponentOutlet prefix

@vicb any thoughts on the issue comment below?

If you do the rename above, then this name is fine.

shlomiassaf · 2016-09-17T00:29:26Z

@vicb almost done, one issue though...
I couldn't pass a test where using the sugared syntax * I can use an @Output on the structural directive.

There is the ngComponentOutletCreated @Output which I couldn't get working on the tests, I couldn't find any reference in the angular code either....

This:

`<div *ngComponentOutlet="currentComponent; providers:providers; created: onOutletCreate($event)"></div>`;

Will error this:

        Can't bind to 'ngComponentOutletCreated' since it isn't a known property of 'div'. ("<div [ERROR ->]*ngComponentOutlet="currentComponent; providers:providers; created: onOutletCreate($event)"

This:
<div *ngComponentOutlet="currentComponent; providers:providers" (ngComponentOutletCreated)="onOutletCreate($event)"></div>;

Won't error but won't trigger... I do expect this to happen as all directive bindings are set within the expression, the context here is the div not the directive.

If this is a limitation I want to note that * syntax also does not allow exportAs so it wont be possible to get the directive as a local template var and subscribe.
You could, I assume, use @Self or @Host to get the directive instance and subscribe.

zoechi · 2016-09-17T10:38:05Z

modules/@angular/common/src/directives/ng_component_outlet.ts

+        }
+
+        const cmpRef = this._viewContainerRef.createComponent(
+            this._cmpFactoryResolver.resolveComponentFactory(this.ngComponentOutlet),


I'm wondering why this doesn't cause issues when't it's called before ngAfterViewInit()?
Isn't ngOnChanges() called before ngAfterViewInit(), at least the first time?
You would obviously have run into issues, so I guess it's safe to assume it's working fine.
Any hint what I might misunderstand?

What issues are you referring to?

this.ngComponentOutlet is checked in line 78, other null issues you see that i'm missing?

I was sure that viewContainerRef.createComponent() can't be used before ngAfterViewInit()and was wondering why it seems to work for you.

@zoechi

This test passes:

it('should work when a component was set before ngOnInit', async(() => { let fixture = TestBed.createComponent(TestComponent); fixture.componentInstance.currentComponent = InjectedComponent; fixture.detectChanges(); expect(fixture.nativeElement).toHaveText('foo'); }));

For sure, ngOnChanges is called here before ngOnInit (thus ngAfterViewInit) and it triggers viewContainerRef.createComponent() since the component exists...

I was not aware of the limitation you mentioned, I experimented on this plunker it looks like this limitation does not exists at this point.

Am i'm missing something?

@shlomiassaf thanks. Seems I'll have to investigate and update my mental model ...

shlomiassaf · 2016-10-06T15:11:17Z

@vicb @vsavkin I need some help with the unsupported feature in angular.
Sugared directives does no support @Output
Should I just ignore that?

mhevery · 2016-10-20T18:11:24Z

Ping @vicb / @vsavkin, can you help with reviewing this PR?

shlomiassaf · 2016-11-08T15:08:54Z

@mhevery / @vicb / @vsavkin,

Gentle reminder on this topic, we are currently block by #12121.

TL;DR:

To support IE we need to support sugar syntax (no template tag)
Sugar syntax does not support @output registration (the blocker...)

Can you advise how to move forward?

Should I ignore the lack of support for @Output when using sugar syntax?

mhevery

I think this is a good PR and should be merged after we get the names right.

mhevery · 2016-11-10T20:02:00Z

modules/@angular/common/src/directives/ng_component_outlet.ts

+ */
+@Directive({selector: 'template[ngComponentOutlet]'})
+export class NgComponentOutlet implements OnChanges {
+  @Input() ngComponentOutlet: Type<any>;


rename to ngOutletComponent

mhevery · 2016-11-10T20:02:52Z

modules/@angular/common/src/directives/ng_component_outlet.ts

+@Directive({selector: 'template[ngComponentOutlet]'})
+export class NgComponentOutlet implements OnChanges {
+  @Input() ngComponentOutlet: Type<any>;
+  @Input() ngOutletInjector: Injector;


If you do the rename above, then this name is fine.

mhevery · 2016-11-10T20:04:47Z

modules/@angular/common/src/directives/ng_component_outlet.ts

+ * @experimental
+ */
+@Directive({selector: 'template[ngComponentOutlet]'})
+export class NgComponentOutlet implements OnChanges {


should be renamed to NgOutlet since Component is a parameter of the outlet and should not be part of it.

shlomiassaf · 2016-11-11T04:01:12Z

@mhevery 👍

I already have a fixed version for sugar syntax but I do have blockers since @Output is not supported.

I will rename the component and post the PR.
Using sugar syntax will be fine but the output wont fire... until #12121 is fixed

luchillo17 · 2016-12-03T13:13:53Z

What has become of this feature?

shlomiassaf · 2016-12-09T20:42:43Z

Sorry guys, long business trip, will get it done hopefully this weekend.

dehru · 2016-12-10T15:15:40Z

Thank you for doing this

mhevery · 2016-12-11T22:05:22Z

I have taken over this PR at #13383

Add NgComponentOutlet directive that can be used to dynamically create host views from a supplied component. Closes angular#11168 Takes over PR angular#11235

Add NgComponentOutlet directive that can be used to dynamically create host views from a supplied component. Closes #11168 Takes over PR #11235

angular-automatic-lock-bot · 2019-09-10T10:21:43Z

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 Sep 1, 2016

vicb reviewed Sep 1, 2016
View reviewed changes

vicb added this to the 2.0.1 milestone Sep 1, 2016

vicb reviewed Sep 1, 2016
View reviewed changes

vicb added the action: review The PR is still awaiting reviews from at least one requested reviewer label Sep 1, 2016

shlomiassaf force-pushed the ng-component-outlet branch from 182a0f0 to 5ccbf09 Compare September 1, 2016 19:11

feat(NgComponentOutlet): add NgComponentOutlet directive

66342e6

This commits adds a new NgComponentOutlet directive that can be used to dynamically create host views from a supplied component type/class. Closes angular#11168

shlomiassaf force-pushed the ng-component-outlet branch from 5ccbf09 to 66342e6 Compare September 1, 2016 19:32

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

vicb suggested changes Sep 16, 2016

View reviewed changes

zoechi reviewed Sep 17, 2016

View reviewed changes

vicb modified the milestones: 2.0.1, 2.0.2, 2.1.0 Sep 29, 2016

vsavkin added the area: common Issues related to APIs in the @angular/common package label Oct 4, 2016

shlomiassaf mentioned this pull request Oct 6, 2016

Sugar (*) syntax does not support @Output (and exportAs) #12121

Open

vicb mentioned this pull request Oct 6, 2016

Proposal: Declarative Dynamic Components #9599

Closed

mhevery reviewed Nov 10, 2016

View reviewed changes

mhevery mentioned this pull request Dec 11, 2016

feat(NgComponentOutlet): add NgComponentOutlet directive #13383

Closed

mhevery closed this Dec 11, 2016

mhevery mentioned this pull request Dec 14, 2016

fix(core): detectChanges() doesn't work on detached instance #13472

Closed

matsko pushed a commit that referenced this pull request Jan 9, 2017

feat(NgComponentOutlet): add NgComponentOutlet directive

8578682

Add NgComponentOutlet directive that can be used to dynamically create host views from a supplied component. Closes #11168 Takes over PR #11235

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

feat(NgComponentOutlet): add NgComponentOutlet directive #11235

feat(NgComponentOutlet): add NgComponentOutlet directive #11235

Uh oh!

Conversation

shlomiassaf commented Sep 1, 2016 • edited by vicb Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shlomiassaf Sep 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shlomiassaf commented Sep 4, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vicb commented Sep 4, 2016

Uh oh!

shlomiassaf commented Sep 6, 2016

Uh oh!

vicb commented Sep 16, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shlomiassaf Sep 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shlomiassaf commented Sep 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shlomiassaf commented Oct 6, 2016

Uh oh!

mhevery commented Oct 20, 2016

Uh oh!

shlomiassaf commented Nov 8, 2016

Uh oh!

mhevery left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shlomiassaf commented Nov 11, 2016

Uh oh!

luchillo17 commented Dec 3, 2016

Uh oh!

shlomiassaf commented Dec 9, 2016

Uh oh!

dehru commented Dec 10, 2016

Uh oh!

mhevery commented Dec 11, 2016

Uh oh!

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

Uh oh!

shlomiassaf commented Sep 1, 2016 •

edited by vicb

Loading

shlomiassaf Sep 1, 2016 •

edited

Loading

shlomiassaf commented Sep 4, 2016 •

edited

Loading

shlomiassaf Sep 22, 2016 •

edited

Loading

shlomiassaf commented Sep 17, 2016 •

edited

Loading