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

CdkPortalOutlet should resolve componentFactories through the portal's ComponentFactoryResolver #9712

Closed
j2L4e opened this issue Jan 31, 2018 · 14 comments · Fixed by #12677 or #13886
Closed
Assignees
Labels
feature This issue represents a new feature or feature request rather than a bug or bug fix P4 A relatively minor issue that is not relevant to core functions

Comments

@j2L4e
Copy link

j2L4e commented Jan 31, 2018

Bug, feature request, or proposal:

At the moment, CdkPortal tries to resolve ComponentFactories via its own CFR which it got from its own Injector instance:

const componentFactory = this._componentFactoryResolver.resolveComponentFactory(portal.component);

It may happen that you'd like to attach a ComponentPortal with a lazily loaded entryComponent that's not resolvable through the PortalHost's CFR. It should then try to get a CFR instance from the Portal to resolve the ComponentFactory if it can't resolve it through its own:

const componentFactory = portal.injector.get(ComponentFactoryResolver).resolveComponentFactory(portal.component);

I'm not sure if this has any negative side effects.

@j2L4e j2L4e changed the title CdkPortalOutlet should resolve components through the portal's ComponentFactoryResolver CdkPortalOutlet should resolve componentFactories through the portal's ComponentFactoryResolver Jan 31, 2018
@jelbourn
Copy link
Member

jelbourn commented Feb 1, 2018

Are you referring to DomPortalOutletor CdkPortalOutlet?

For the first, it uses whatever ComponentFactoryResolver it is given (it's not injected). When used from the overlay, it will be the resolver associated w/ the overlay service, which is expected since the OverlayModule needs to be imported in any lazy-loaded module that would be using it.

For the second, it will use the ComponentFactoryResolver from wherever in the injection tree the outlet lives. The only way I would see this being a problem is if the ComponentPortal is for a component defined in a sibling module. Is that what you're seeing?

@j2L4e
Copy link
Author

j2L4e commented Feb 5, 2018

I'm talking about CdkPortalOutlet.
And yes, this is about lazily loaded children or siblings.

My specific use case is a navigation menu outlet:
<ng-container menuOutlet="topnav"></ng-container> with MenuOutlet extending CdkPortalOutlet

Modules can bring in their own navigation components through route data. Obviously the outlet lives somewhere higher up in the hierarchy and doesn't know about SomeTopnavComponent's factory when it's part of a lazily loaded module.

  {
    path: 'somePath',
    component: SomeComponent,
    data: {
      topnav: SomeTopnavComponent
    },
  }

It'd be great to be able to pass a ComponentFactoryResolver to CdkPortalOutlet in case one needs to deal with sibling and child modules.

@jelbourn jelbourn added feature This issue represents a new feature or feature request rather than a bug or bug fix P4 A relatively minor issue that is not relevant to core functions labels Feb 5, 2018
@crisbeto crisbeto self-assigned this Feb 10, 2018
crisbeto added a commit to crisbeto/material2 that referenced this issue Feb 10, 2018
…dkPortalOutlet

Adds the ability for consumers to pass in a different `ComponentFactoryResolver` when attaching to a `CdkPortalOutlet` programmatically.

Fixes angular#9712.
crisbeto added a commit to crisbeto/material2 that referenced this issue Feb 12, 2018
…dkPortalOutlet

Adds the ability for consumers to pass in a different `ComponentFactoryResolver` when attaching to a `CdkPortalOutlet` programmatically.

Fixes angular#9712.
@j2L4e
Copy link
Author

j2L4e commented Jun 11, 2018

@crisbeto you closed the linked PR stating that this wouldn't be necessary in Angular 6+.
Could you elaborate on how resolving factories has changed?

Thank you for your work!

@poke
Copy link

poke commented Aug 10, 2018

@crisbeto I have the same question as @j2L4e.

Basically, as per angular/angular#14324, I am unable to attach component portals to my shared portal outlet when the component I want to render is declared within a lazy loaded module. Since the portal outlet’s component factory resolver does not know about the entry components from lazy loaded modules, I need to use the module’s component factory resolver.

So I need a way to be able to pass the component factory resolver to the component portal, or the portal outlet’s attach method.

The pull request did exactly that, and I can confirm (through monkey patching) that this indeed works for me. But even with Angular 6.1, I do not see how this is possible without being able to pass the factory; or without the outlet retrieving the factory from the injector that is passed through the portal.

Could you clarify why you believe that the changes from that PR are no longer necessary for Angular 6? I might be wrong, but I haven’t been able to find a way around the component factory resolver.

@crisbeto
Copy link
Member

@poke If I remember correctly, the "Angular 6+" changes that I mentioned when closing the PR were Ivy, but that hasn't landed yet. @jelbourn do you think we should revisit this?

@jelbourn
Copy link
Member

Probably. Have an API in mind?

@crisbeto
Copy link
Member

I was thinking of basically doing the same as in #9882 which we closed a while ago, because of Ivy.

@poke
Copy link

poke commented Aug 13, 2018

For what it’s worth, I am using the exact same strategy as in that pull request in a patched version running on 6.1 and that works completely fine. Having to pass an explicit component factory resolver to the attach seems like a fine compromise to make this work.

@jelbourn
Copy link
Member

Why make the resolver a parameter to attach instead of an @Input on the directive?

@poke
Copy link

poke commented Aug 13, 2018

The point of this is that you can attach various component portals to the same outlet. So you can attach compontent X of lazy-loaded module A using A’s component factory resolver, and in the next moment you can attach component Y of lazy-loaded module B using B’s component factory resolver. So the component factory resolver you choose is always strongly coupled to the component portal you want to attach.

If this was an input, then you would have to move this logic to the parent component, and you would be pretty much defeating the purpose of component portals and the attach API. Because at that point, you could also just switch to template portals.

The component factory resolver being coupled to the component portal would probably be an argument for moving the component factory resolver reference into the component portal, instead of passing it while attaching.

At that point, this might be an argument for @j2L4e’s original idea, to resolve the component factory resolver from the component portal’s injector. I don’t know if that actually works, but from my experiments, passing the injector to the component portal is not required to make it work. I have been using the outlet’s injector with a component from a different NgModule using that module’s component factory resolver, and that’s been working very well. So if the component factory resolver is moved to the component portal (instead of the attach), it should probably still be an explicit constructor argument or property.

@jelbourn
Copy link
Member

That's a good point; you'd want the resolver to be associated with the ComponentPortal in that case. That makes more sense anyway, since TemplatePortal doesn't use the resolver at all. The simplest thing would probably be adding a ComponentFactoryResolver as a 4th argument to the constructor for ComponentPortal. I would shy away from trying to get it from the given injector because someone might want to use an injector from a totally different part of the application tree.

@crisbeto crisbeto removed the has pr label Aug 14, 2018
crisbeto added a commit to crisbeto/material2 that referenced this issue Aug 14, 2018
…ted with portal

Allows consumers to specify a their own `ComponentFactoryResolver` when creating a `ComponentPortal`. Previously we would only use the resolver from the `PortalOutlet`.

Fixes angular#9712.
jelbourn pushed a commit that referenced this issue Aug 21, 2018
…ted with portal (#12677)

Allows consumers to specify a their own `ComponentFactoryResolver` when creating a `ComponentPortal`. Previously we would only use the resolver from the `PortalOutlet`.

Fixes #9712.
@poke
Copy link

poke commented Oct 30, 2018

I just updated my application to Angular 7 and cdk 7.0.2 and wanted to migrate to this solution. However, I realized that the change that now made it into the release only applies to the DomPortalOutlet. When using the CdkPortalOutlet, which is what this issue was originally about, there is still the same component factory resolver being used.

So unfortunately, the fix for this does not work for me. Any chance we can get this ported to the CdkPortalOutlet as well?

@jelbourn
Copy link
Member

Re-opening this for the change still needed in CdkPortalOutlet

@jelbourn jelbourn reopened this Oct 30, 2018
crisbeto added a commit to crisbeto/material2 that referenced this issue Oct 30, 2018
…irective

Follow-up from angular#12677 where we missed to add the new functionality to the `CdkPortalOutlet` directive.

Fixes angular#9712.
jelbourn pushed a commit that referenced this issue Nov 3, 2018
…irective (#13886)

Follow-up from #12677 where we missed to add the new functionality to the `CdkPortalOutlet` directive.

Fixes #9712.
atscott pushed a commit to atscott/components that referenced this issue Nov 5, 2018
…irective (angular#13886)

Follow-up from angular#12677 where we missed to add the new functionality to the `CdkPortalOutlet` directive.

Fixes angular#9712.
jelbourn pushed a commit that referenced this issue Nov 6, 2018
…irective (#13886)

Follow-up from #12677 where we missed to add the new functionality to the `CdkPortalOutlet` directive.

Fixes #9712.
@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 10, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature This issue represents a new feature or feature request rather than a bug or bug fix P4 A relatively minor issue that is not relevant to core functions
Projects
None yet
4 participants