-
Notifications
You must be signed in to change notification settings - Fork 25.1k
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
feat(NgComponentOutlet): add NgModule support to NgComponentOutlet directive #14088
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed, please reply here (e.g.
|
fcce7bc
to
6276120
Compare
CLAs look good, thanks! |
@@ -20,16 +21,20 @@ import {ComponentFactoryResolver, ComponentRef, Directive, Injector, Input, OnCh | |||
* | |||
* You can control the component creation process by using the following optional attributes: | |||
* | |||
* * `ngOutletInjector`: Optional custom {@link Injector} that will be used as parent for the | |||
* * `ngComponentOutletInjector`: Optional custom {@link Injector} that will be used as parent for | |||
* the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
formatter mess up, please join lines.
@@ -57,16 +63,16 @@ import {ComponentFactoryResolver, ComponentRef, Directive, Injector, Input, OnCh | |||
* @experimental |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add example for Module
@@ -42,7 +47,8 @@ import {ComponentFactoryResolver, ComponentRef, Directive, Injector, Input, OnCh | |||
* ``` | |||
* <ng-container *ngComponentOutlet="componentTypeExpression; | |||
* injector: injectorExpression; | |||
* content: contentNodesExpression"> | |||
* content: contentNodesExpression; | |||
* module: moduleExpression;"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rename to ngModuleFactory
* component. | ||
* | ||
* * `ngOutletContent`: Optional list of projectable nodes to insert into the content | ||
* * `ngComponentOutletContent`: Optional list of projectable nodes to insert into the content | ||
* section of the component, if exists. ({@link NgContent}). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove @link
since there is nothing such as NgContent
@@ -78,9 +84,19 @@ export class NgComponentOutlet implements OnChanges { | |||
if (this.ngComponentOutlet) { | |||
let injector = this.ngComponentOutletInjector || this._viewContainerRef.parentInjector; | |||
|
|||
if (this.ngComponentOutletModule) { | |||
this.moduleRef = this.ngComponentOutletModule.create(injector); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- make sure that
moduleRef
does not have value and destroy it. - Guard this on module change only.
changes.ngComponentOutletModule
@Input() ngComponentOutlet: Type<any>; | ||
@Input() ngComponentOutletInjector: Injector; | ||
@Input() ngComponentOutletContent: any[][]; | ||
@Input() ngComponentOutletModule: NgModuleFactory<any>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Turn this into a setter and create/destroy module on setter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we decided not to do it that way, right? Changed to clean up moduleRef at the right time.
expect(moduleRef.destroy).not.toHaveBeenCalled(); | ||
fixture.destroy(); | ||
expect(moduleRef.destroy).toHaveBeenCalled(); | ||
})); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add one more test that updating a type should not rebuild module injector.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added two tests, one for updating the component and one for updating component and module.
…rective Allow NgComponentOutlet to dynamically load a module, then load a component from that module. Useful for lazy loading code, then add the lazy loaded code to the page using NgComponentOutlet. Closes angular#14043
6276120
to
13f2b6b
Compare
…rective (angular#14088) Allow NgComponentOutlet to dynamically load a module, then load a component from that module. Useful for lazy loading code, then add the lazy loaded code to the page using NgComponentOutlet. Closes angular#14043
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
…rective
Allow NgComponentOutlet to dynamically load a module, then load a component from
that module. Useful for lazy loading code, then add the lazy loaded code to the
page using NgComponentOutlet.
Closes #14043
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (check one with "x")
What is the current behavior? (You can also link to an open issue here)
What is the new behavior?
Does this PR introduce a breaking change? (check one with "x")
If this PR contains a breaking change, please describe the impact and migration path for existing applications: ...
Other information: