shlomiassaf / ngx-modialog

Modal / Dialog for Angular
http://shlomiassaf.github.io/ngx-modialog
MIT License
686 stars 242 forks source link

Provide ability to open dynamically created components #200

Open herwix opened 8 years ago

herwix commented 8 years ago

I need to create a modal component from a module which I have dynamically compiled in the app. It would be awesome if I could simply provide the proper injector that will be used for the creation of the custom modal component or alternatively supply a componentFactory that can create the custom modal component.

I have looked trough the source code and I have not found an easy way to do this. I could probably subclass the overlayRenderer to achieve what I want to do, but there is not really good documentation on how to do that in a way that would be aligned with the intentions of the module (e.g., for compatibility with future versions, etc.)

modal.open() also accepts a componentFactory

or

overlayConfigFactory optionally also accepts an injector that will be used to create the custom component (not the overlay itself).

Dynamically compiled modules with modal components should be displayable in the main application.

JulioC commented 8 years ago

I'm also having issues when trying to open component from dynamically compiled ngModules.

I haven't investigated it yet, but I believe the service on angular2-modal that creates the Component is hierarchically above the loaded module and therefore can't find their component factories.

Both solutions from @herwix looks fine to me

shlomiassaf commented 8 years ago

I'm not sure what you mean...

the open method of the Modal service accepts either a Component, TemplateRef or a string. You can also attach bindings in the overlayConfig.

herwix commented 8 years ago

I think that JulioC has the right idea. Basically when you dynamically compile a module than you are possibly loading new components that have not been registered with the hierarchical injector in the main app component (where the defaultOverlay is located). So when you try to open that component it cannot be found by the modal service because the hierarchical injector does not know about the components in the dynamic compiled module. At leasts that's my thinking...

I think that bindings don't work here because that only works for providers and not components, right?

I have been trying to work around that by using the injector of the application as a parent injector for the compilation of my dynamically compiled module and using the overlayTarget directive in a dynamically compiled module so that I can reference the overlayTarget by name and it should automatically use the appropriate injector for the creation of the modal content. But I have not gotten it running as of yet.

I hope the use case is a little bit clearer now. If you have any idea how to make it work that would be greatly appreciated :) And thanks for the effort with the module, it's really helpful and I am using it a lot :)

shlomiassaf commented 8 years ago

@herwix You can supply bindings in the OverlayConfig, i'm not sure it will help since I don't really know what is the token for a component, maybe it's just the component class maybe is a multi provider with a list of components... dont know.

Can you try setting the defaultViewContainer via template and not programatically.

<span defaultOverlayTarget></span>

See this example

It's kinda like the overlayTarget attempt you try to run but with less places to make mistakes.

The template/component hosting the

<span defaultOverlayTarget></span>

Must have an injector that already has the dynamic components registered.

herwix commented 8 years ago

@shlomiassaf I have used the defaultOverlayTarget in my root app component and that works fine. I still need the modals to work there. So, I wanted to use the overlayTarget to create additional modal outlets for my dynamic components (e.g., register them in the vcRefStore) in a place where I have the right injector. I don't think the directive is being picked up though at the moment. Kinda difficult to debug when it's not your own component though :D. I am close to managing the vcRefStore myself with a copy of the targetOverlay directive to see how things work then...

To make it work I also need dynamic bindings of names in the vcRefStore so I will probably have to do that anyway.

Everything would be a lot easier if we could simply provide the right injector though and don't have to make the workaround via viewContainerRefs ;) Then we could still keep using the defaultOverlayTarget but also create components that come from a dynamically compiled module.

Following would be the method that allows you to create components via viewContainerRef with a custom injector (Method in ViewContainerRef).

abstract createComponent<C>(componentFactory: ComponentFactory<C>, index?: number, injector?: Injector, projectableNodes?: any[][]): ComponentRef<C>;

herwix commented 8 years ago

So I have been able to get the right viewContainerRef (with proper injector set) and set it as the viewContainer on the overlayConfig. Now no errors are thrown about not recognizing the component, but the component is still not rendered.

I don't really see why that is happening, though. Any ideas of what might go wrong?

JulioC commented 8 years ago

As I understand, setting viewContainer will make the overlay appear inside the given element.

If this interpretation is correct, that is not the desired behavior for my case.

I'll try to provide a plunkr illustrating the problem.

shlomiassaf commented 8 years ago

@herwix The DOMOverlayRenderer is using ComponentFactoryResolver, I believe it's injected before your dynamic components are registered.

Now, ComponentFactoryResolver. resolveComponentFactory is not getting any injector or ViewContainerRef so my guess is that it's still using the old Injector, but I might be wrong... probably wrong :)

herwix commented 8 years ago

@shlomiassaf so I finally got it working... the problem was that somehow changeDetection wasn't running on the container after it was created... strangely only with components from a different viewContainerRef. Might not be the complete solution but for me it seems to work right now.

My 'fix' is in my fork. I don't know if it's worthwhile to make a pull request for that. But would be awesome if that could be shipped in the next release.

To sum up my approach:

get a viewContainerRef from a dynamically compiled module and set that manually on the overlayConfig.viewContainer. Then everything should work as expected.

shlomiassaf commented 8 years ago

@herwix

Calling dialogRef.overlayRef.instance.addComponent<T>(ContainerComponent, b, nodes); includes a call to changeDetectorRef.detectChanges(); source code

So, detectChanges won't cut it, you must also call markForCheck. So this is where it get's tricky, it can be many things since we are playing with the view tree.

The 1st call is to detectChanges, this is a synced call, when it's done we mark to change for changes and this will happen on the next vm turn.

I would like to know the reason, so I know if to add it in the addComponent function or just leave it in the modal.

Can you please put up a plunker the illustrates the error, something simple with dynamic compilation. I will try to debug the root cause, should give a good insight on CD as well.

Thanks.

herwix commented 8 years ago

@shlomiassaf I am sorry but I am out until late next week. And I don't know if I will have any more time to spend on this, already spent the whole day... maybe @JulioC can set up a case to demonstrate his use case and we can test if the fix is working there as well?

JulioC commented 8 years ago

I've made this plunkr illustrating the issue.

AppComponent is the defaultOverlayTarget, its module imports both ModalModule.forRoot() and BootstrapModalModule. It will compile OtherModule and create OtherComponent in the view.

OtherComponent uses Modal service to open a OtherModalComponent, but fails with:

Error in ./ModalOverlay class ModalOverlay - inline template:2:14 caused by: No component factory found for OtherModalComponent

I tried following the calls inside Modal.open, but I was unable to identify where it creates the content component.

If there is any mistake on my code, please point it out.

herwix commented 8 years ago

Thanks for the plunker! Just to demonstrate my workaround: plunker

The modal will still be inserted in the same place as before but receive the right injector via the new viewContainerRef. It's not optimal because you have to have a target within the target module but for me that's possible, so I could live with that for the moment. Interestingly, the cdr.markForCheck() does not need to be called here. For me it would simply render the modal frame but not the content because the swapComp directive was not receiving its input properties.

JulioC commented 8 years ago

I tried your workaround on my application code and somehow it doesn't work.

This part of my application is still really ugly after migrating to ngModules, so I'll rewrite it on the next days. Maybe I can find out why it's not working.

herwix commented 8 years ago

If the moda shows up but there is no content (and no error) it might be the same refresh problem that I faced. I also wrote a custom overlaytarget directive which also implements OnChanges but that might not be necessary.

Am 24.09.2016 um 17:07 schrieb Júlio César notifications@github.com:

I tried your workaround on my application code and somehow it doesn't work.

This part of my application is still really ugly after migrating to ngModules, so I'll rewrite it on the next days. Maybe I can find out why it's not working.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

JulioC commented 8 years ago

It throws the same exception as before, so I don't think it's the same case as yours.

shlomiassaf commented 8 years ago

@herwix @JulioC Thanks for the plunkers. I'll review it and see what's the best approach to introduce support for dynamically compiled components.

JulioC commented 8 years ago

I refactored the way I lazy load my modules and now the workaround is doing it for me. Sadly I wasn't able to identify what changed fixed it.

Edit:

Wow, just realized I had made another temporary solution and didn't remove it on my last tests:

The workaround posted here still does not work for me. I ended up including the Modal components on the statically loaded module tree.

Anyway, thanks for the help from both of you! 👍

herwix commented 8 years ago

Just to note for my implementation the call to mark for check would still need to be 'fixed' otherwise my dynamic component is not swapped in. I don't know why though. In the plunker it seems to work fine... but I somehow doubt it is really related to my code because the call in question is before my component gets rendered. (Maybe I am using it in an OnPush part of the component tree and that causes problems?)

Cheers, alex

Am 25.09.2016 um 19:23 schrieb Júlio César notifications@github.com:

I refactored the way I lazy load my modules and now the workaround is doing it for me. Sadly I wasn't able to identify what changed fixed it.

I'll keep watching for you solution, but it's not that urgent anymore.

Thanks @shlomiassaf @herwix

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

charsleysa commented 8 years ago

@JulioC @herwix I had a similar problem.

I tried to open a modal in a lazy loaded module and I was getting the No component factory found for xxxxxx error. I couldn't for life of me figure out why.

My workaround was to setup the main app component like this:

export class App {
    constructor(
        private overlay: Overlay,
        private viewContainerRef: ViewContainerRef
    ) {
        overlay.defaultViewContainer = viewContainerRef;
    }
}

Then, inside my component which resides in the lazy loaded module, I did this (some app specific code omitted):

export class ContentPage {
    constructor(
        private modal: Modal,
        private router: Router,
        private route: ActivatedRoute,
        private viewContainerRef: ViewContainerRef
    ) { }

    public changeLocation() {
        this.modal.open(ChangeLocationDialog, overlayConfigFactory({}, VEXModalContext, { viewContainer: this.viewContainerRef }));
    }
}

This works and places the modal directly inside the body tag and has no rendering issues for me.

Hope this helps!

shlomiassaf commented 8 years ago

@JulioC @herwix Didn't forget about this, had to focus on AoT support so project with AoT (all ng-cli's) will be able to use the library. Major change to build process.

I will get to this :)

shlomiassaf commented 8 years ago

@JulioC @herwix Here's my first step towards support, allow supplying injector to OverlayConfig.

The injector is optional, if not supplied the injector is taken from the ViewContainerRef.

Something like this:

          return this.compiler.compileModuleAsync(RuntimeCompiledModule)
            .then(moduleFactory => moduleFactory.create(this.injector))
            .then(module => {
              return this.modal.open(RuntimeCompiledComponent, overlayConfigFactory({}, BSModalContext, {
                injector: module.injector
              }));
            });

The overlay is created with the supplied injector which means it's inner ViewContainerRef will inherit from that injector. Since all child components inside an overlay are using that ViewContainerRef it will work without issues.

One edge case that came in mind is when ViewContainerRef is set explicitly in OverlayConfig, if no injector is set and the Component to render is lazy/runtime compiled then it will throw.

So, if you set a view container that was created before a component was registered you also need to supply an injector.

A solution to workaround that, as well as @charsleysa issue, is to re-set the Overlay. defaultViewContainer to a new value once a lazy loaded module was loaded.

Anyway, having runtime compiled component and lazy loaded components is an advanced scenario since DI get's mixed up. This is all new so maybe other things might be of help, maybe having a default injector and being able to change that so every module created (runtime or lazy) can set that default injector. This is tricky tough it might mess DI Hierarchy and developers won't understand why some dependencies are inject incorrectly. To solve this the best practice should be setting a default injector every time a new compilation is done but not using injectors in the OverlayConfig, use bindings so the injector can be easily extended. Anyway this is new and advanced so I won't allow default injectors for now, but let's see if this does help.

shlomiassaf commented 8 years ago

@JulioC regarding you note about supplying a ViewContainerRef (string or actual). If supplied within a call to modal.open() it will not default to display blocked inside the view container boundaries. i.e: It will not be inside the element.

To display inside an element you must call inElement(true)

charsleysa commented 8 years ago

@shlomiassaf thanks for your work!

I think in my situation with lazy loaded components, passing the injected ViewContainerRef of the component that is instantiating the modal is the best way to do it following your comments as I don't have an instance of the Injector.

shlomiassaf commented 8 years ago

@herwix checkout the demo site in with the bootstrap plugin.

There is a JIT button, it will also allow you to see insertion of JIT compiled component into a previously created view view container ref.

Still not in master, it's in the dynamic-compilation branch. The demo site represents a build from this branch

shlomiassaf commented 8 years ago

@herwix @JulioC @charsleysa Did you test it, can we close this?

JulioC commented 8 years ago

The injector config did it for me. Thanks!

charsleysa commented 8 years ago

@shlomiassaf I'm unsure as to the difference between injector and viewContainer as they both seem to be doing the same thing.

But to answer you question, yes it seems to be working, although I'm just using the injector from the viewContainer:

export class ContentPage {
    constructor(
        private modal: Modal,
        private router: Router,
        private route: ActivatedRoute,
        private viewContainerRef: ViewContainerRef
    ) { }

    public changeLocation() {
        this.modal.open(ChangeLocationDialog, overlayConfigFactory({}, VEXModalContext, { injector: this.viewContainerRef.injector }));
    }
}
herwix commented 8 years ago

@shlomiassaf I am currently on vacation, so I am sorry but I won't have time to check until late next week. But as I remember the error that I was describing was actually not really related to the injector but the change detection. So it would be nice to keep this open until I can recheck :)

JulioC commented 8 years ago

@charsleysa you can inject Injector directly, which is the same as the one from ViewContainerRef I believe.

vijender1256 commented 7 years ago

@herwix @charsleysa Did anyone fix this issue , I am having the same issue :

image

ConclusionCodeWindow is my component.

Here is my code for Module:

import { NgModule }       from "@angular/core";
import { CommonModule }   from "@angular/common";
import { CashierRiskProfileComponent }    from "./cashierriskprofile.component";
import { CashierRiskProfileService }   from "./cashierriskprofile.service";
import { CashierRiskProfileRouting } from "./cashierriskprofile.routing";
import { SharedModule } from "../shared/shared.module";
import { ModalModule } from 'angular2-modal';
import { BootstrapModalModule } from 'angular2-modal/plugins/bootstrap';
import { ConclusionCodeWindow } from './conclusioncode.modal';

@NgModule({
    imports: [
        CommonModule,
        SharedModule,
        CashierRiskProfileRouting,
        ModalModule.forRoot(),
        BootstrapModalModule
    ],
    declarations: [
        CashierRiskProfileComponent,ConclusionCodeWindow
    ],
    providers: [
         CashierRiskProfileService
    ]
})
export class CashierRiskProfileModule { }

and for Component here is my code:

import { Component, ViewContainerRef, OnInit, Output, EventEmitter, ViewChild } from "@angular/core";
import { Router, ActivatedRoute } from "@angular/router";
import { Modal } from 'angular2-modal/plugins/bootstrap';
import { ConclusionCodeWindow, ConclusionCodeWindowData } from './conclusioncode.modal';
import { Overlay } from 'angular2-modal';

   constructor(overlay: Overlay, vcRef: ViewContainerRef, public modal: Modal) { overlay.defaultViewContainer = vcRef; }

  onStatusChange(status) {
        this.modal.open(ConclusionCodeWindow, new ConclusionCodeWindowData(status));
    }

I am getting error No component factory found

Below is the plunker demostration, I see error in console window:

https://plnkr.co/edit/ZeRajGkGwyvKRIZkG4XQ?p=preview