fix(modal): support server rendering with lazy loading #2180
Conversation
src/modal/modal.ts
Outdated
|
|
||
| /** | ||
| * Opens a new modal window with the specified content and using supplied options. Content can be provided | ||
| * as a TemplateRef or a component type. If you pass a component type as content than instances of those | ||
| * components can be injected with an instance of the NgbActiveModal class. You can use methods on the | ||
| * NgbActiveModal class to close / dismiss modals from "inside" of a component. | ||
| */ | ||
| open(content: any, options: NgbModalOptions = {}): NgbModalRef { | ||
| return this._modalStack.open(this._moduleCFR, this._injector, content, options); |
There was a problem hiding this comment.
This is not good, as it would change ComponentFactoryResolver instance and break lazy-loaded modules (I could not open a component from a lazy-loaded module).
There was a problem hiding this comment.
Thanks for the tip!
I was looking for how ComponentFactoryResolver and now makes sense why the ComponentFactoryResolver was here!
src/modal/modal-stack.ts
Outdated
| // Cant build factories on constructor when using lazy loading | ||
| private _buildFactories() { | ||
| this._buildBackdropFactory(); | ||
| this._buildWindowFactory(); |
There was a problem hiding this comment.
I don't think that caching factories here, in the code, adds any performance benefits. Remove those methods and replace with a direct call to this._componentFactoryResolver.resolveComponentFactory(NgbModalBackdrop)
src/modal/modal-stack.ts
Outdated
|
|
||
| if (!containerEl) { | ||
| throw new Error(`The specified modal container "${containerSelector}" was not found in the DOM.`); | ||
| throw new Error(`The specified modal container "${options.container}" was not found in the DOM.`); |
There was a problem hiding this comment.
This will have a broken error message if a user doesn't specify the container option
There was a problem hiding this comment.
If a user doesn't specify the container option then will use this._document.body and not a query selector.
this._document.body will be always defined, right?
There was a problem hiding this comment.
this._document.body will be always defined, right?
In theory not always, as it can potentially be null: https://developer.mozilla.org/en-US/docs/Web/API/Document/body
In practice it should be always defined, but it is better to be safe than sorry...
src/modal/modal-stack.ts
Outdated
| private _getContentRef( | ||
| moduleCFR: ComponentFactoryResolver, contentInjector: Injector, content: any, | ||
| context: NgbActiveModal): ContentRef { | ||
| private _getContentRef(contentInjector: Injector, content: any, context: NgbActiveModal): ContentRef { |
There was a problem hiding this comment.
The signature should stay as-is, we need to use a proper ComponentFactoryResolver
|
@Scoup thnx for the PR. I did the first pass on it and left the comments in the code, here is the gist:
Please resolve those issues and I will have another look. |
Scoup
force-pushed
the
fix-modal-server-side
branch
from
93c06e2
to
f99bc9d
Compare
Scoup
force-pushed
the
fix-modal-server-side
branch
from
f99bc9d
to
8b29911
Compare
|
@pkozlowski-opensource I did the changes. Was very nice to learn how ComponentFactoryResolver works behind the scene. Thanks helping me. |
Closes #1968
Fixes #858
This PR fix the problem
No component factory found for NgbModalBackdrop. Did you add it to @NgModule.entryComponents? at noComponentFactoryErrorwhen using ng-bootstrap on server side.We had 2 main issues:
NgbModalWindowandNgbModalBackdropare not on the factories list yet. For that reasonComponentFactory<NgbModalBackdrop>andComponentFactory<NgbModalWindow>will be initialized only whenopenis called.documentaccess directly. I changed the code so we can inject the document to run on server side.