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

feat(Modal): allow modal to be lazily loaded from a module on demand #772

Merged
merged 18 commits into from
May 25, 2017
Merged

feat(Modal): allow modal to be lazily loaded from a module on demand #772

merged 18 commits into from
May 25, 2017

Conversation

NathanWalker
Copy link
Contributor

@NathanWalker NathanWalker commented Apr 30, 2017

  • Allows a NgModule to be lazily loaded on-demand (ie, via tap of a button without a route config) using the NSModuleFactoryLoader
  • Allows a Component to be opened in a modal even if it was lazily loaded on demand
  • Does not disrupt any current api's
  • No breaking changes
  • Provides tests for this feature (could be cleaned up with some help)
  • Also cleans up template usage throughout tests to use Angular 4 ng-template

Assuming usage of the following in the module and/or a component's providers:

  ...
  providers: [
    { provide: NgModuleFactoryLoader, useClass: NSModuleFactoryLoader }
  ]
  ...

Example Usage in a Component:

constructor(
  private moduleLoader: NgModuleFactoryLoader, 
  private vcRef: ViewContainerRef,
  private modalService: ModalDialogService
) {}

public buttonTap() {
  // could be wired to any button (tap) event in a component view, etc.

  // Lazily load a module, gain access to a component from that new module
  // immediately open the new lazily loaded component in a modal
  (<NSModuleFactoryLoader>this.moduleLoader)
    .loadAndCompileComponents('./lazy-loaded.module#LazyModule')
    .then((mod: ModuleWithComponentFactories<any>) => {
      const moduleRef = mod.ngModuleFactory.create(this.vcRef.parentInjector);
      // find component factory to ref the correct componentType
      let lazyCmpFactory: ComponentFactory<any>;
      for (let cmp of mod.componentFactories) {
        if (cmp.selector === "modal-lazy-comp") { // find by Component's selector
          lazyCmpFactory = cmp;
          break;
        }
      }
                    
      this.modalService.showModal(lazyCmpFactory.componentType, {
        moduleRef,
        viewContainerRef: this.vcRef,
        context: {}
      });
  });
}

/cc @vakrilov

@vakrilov
Copy link
Contributor

vakrilov commented May 1, 2017

LGTM!
Here is something I'm wondering: Do you know if compileModuleAndAllComponentsAsync has some internal caching logic? For sure - we don't want calling the same dialog twice to load and compile the module and all components inside twice. I guess if the module and factories are cached the second time the dialog is shown should be considerably faster.

@NathanWalker
Copy link
Contributor Author

@vakrilov good question - I will take a look and see (I was assuming so but will confirm!)

@NathanWalker
Copy link
Contributor Author

Looks like there is an internal compiled ngmodule cache Angular uses under the hood:
https://github.com/angular/angular/blob/09d9f5fe54e7108c47de61393e10712f8239d824/packages/compiler/src/jit/compiler.ts#L122
👍

@vakrilov
Copy link
Contributor

vakrilov commented May 2, 2017

Those angular guys rock!

@sis0k0
Copy link
Contributor

sis0k0 commented May 8, 2017

Hey, @NathanWalker!

Awesome PR! Can you attach a project with example usage, so we can integrate it in our e2e tests?

@NathanWalker
Copy link
Contributor Author

@sis0k0 Thanks - the PR contains tests: https://github.com/NathanWalker/nativescript-angular/blob/lazy-load-then-open-modal/tests/app/tests/modal-dialog.ts#L145-L175 - this is the example usage however I can provide a completely separate repo with the same thing?

@sis0k0
Copy link
Contributor

sis0k0 commented May 10, 2017

Yes, that would be great! :)

@NathanWalker
Copy link
Contributor Author

@sis0k0 I added yourself and @vakrilov to this test repo:
https://github.com/NathanWalker/lazy-modal
Keep in mind it's package was targeting my local copy of this repo containing this PR:
https://github.com/NathanWalker/lazy-modal/blob/master/package.json#L21
Feel free to clone and modify as needed.
This demonstrates the features presented in this PR:

  • Ability to reuse a lazily loaded module's component as a routed component or...
  • Ability to open that same component as a lazily loaded modal

ItemComponent contains 2 buttons which demonstrate this:
https://github.com/NathanWalker/lazy-modal/blob/master/app/item/items.component.html#L4-L5
Which opens this component via lazy loaded route or via lazy loaded modal:
https://github.com/NathanWalker/lazy-modal/blob/master/app/lazy/components/lazy.component.html
Various bindings exist for you to setup integration tests around this feature appropriately.

@NathanWalker
Copy link
Contributor Author

The failing test appears to be something related to unmet peer dep - lemme know if something you can reset on your end to rebuild - the PR was previously passing before merging in latest upstream master.

@sis0k0
Copy link
Contributor

sis0k0 commented May 15, 2017

Hey, @NathanWalker,
I've got a few questions about the PR :)

  1. If you want to load a component in the above way, you cannot inherit the providers from the component's original module.
    That means all injected things in lazy component should be provided either directly in its providers or somewhere in the providers of the loader component.

For example: you inject RouterExtensions in the LazyComponent and provide them in the LazyModule, but you don't have such provider in AppModule.
Then you try to load LazyComponent inside AppComponent, but that fails because LazyComponent cannot find provider for RouterExtensions.

However, that's not such a big problem. We can add a catch here with some error message for that case. What do you think?

  1. Let's say you Ahead-of-Time compiled your app with webpack. If you use the loadAndCompile method of the NativeScriptModuleFactoryLoader, it'll try to require the module from the provided path (string). But since we bundled the app in one file (or several), the file won't exist. You'll get the following error:
JS: ERROR Error: com.tns.NativeScriptException: Failed to find module: "/data/data/org.nativescript.lazymodal/files/app/./lazy/lazy.module", relative to: app//

Now, when we're the AoT compiler, we're invoking the default Angular module loader SystemJsNgModuleLoader, which internally loads the module with System.import. The @ngtools/webpack plugin finds all lazy loaded modules that were declared with loadChildren and creates chunks for each of them.
We can try to do the same, but we'll have to traverse the whole source code, cause the module can be lazy loaded anywhere in the code. That also won't work for the cases when we build the path for the module (which is a string) dynamically at runtime and don't know which module to extract at build time. Have you thought about that feature in the context of AoT compilation?

@NathanWalker
Copy link
Contributor Author

NathanWalker commented May 17, 2017

Hi @sis0k0 n/p.

For example: you inject RouterExtensions in the LazyComponent and provide them in the LazyModule, but you don't have such provider in AppModule. Then you try to load LazyComponent inside AppComponent, but that fails because LazyComponent cannot find provider for RouterExtensions.

  1. That's purely a developer issue. Angular's docs clearly state singleton provider issues as they relate to lazy loading and otherwise here:
    https://angular.io/docs/ts/latest/guide/ngmodule.html
    and very well further clarified here:
    https://angular.io/docs/ts/latest/cookbook/ngmodule-faq.html

The catch you mentioned in my opinion is not something I would handle in this plugin - Angular has it's own error condition handling which properly throws on this case.

Let's say you Ahead-of-Time compiled your app with webpack...We can try to do the same, but we'll have to traverse the whole source code, cause the module can be lazy loaded anywhere in the code.

  1. Great question. Perhaps we could provide an elegant way to indicate which modules are lazy loaded in addition to the default route config (in the particular case where a developer intends to purely lazy load a modal which does not also have a routing config for).

Please consider this simplicity:

// we could create this new InjectionToken
import { LazyLoadedModals } from 'nativescript-angular/platform';

// root app module
@NgModule({
  ...
  providers: [
    { 
      provide: LazyLoadedModals,
      useValue: [
        // all lazy loaded modals used explicitly with loadAndCompileComponents...
        './modules/sample.module#SampleModule',
        './modules/another.module#AnotherModule',
        // etc.
      ]
    }
  ]
})

This can be used to help webpack create the bundle/chunk with the help of nativesript-dev-webpack. Then we (or I) simply document this fact to provide this value to the token in the very particular case of a user lazy loading a modal (with no routing config to the same module).

Then at this point, it's just a developer and documentation thing. No hooks or error condition handling needed due to same reasons stated for 1.

Thoughts?

@sis0k0
Copy link
Contributor

sis0k0 commented May 19, 2017

@NathanWalker
Copy link
Contributor Author

@sis0k0 Thank you so much, love your changes! 👍 👍 👍

@sis0k0
Copy link
Contributor

sis0k0 commented May 23, 2017

@NathanWalker,

Great! We'll fix the failing build ;) Meanwhile, can you drop the commit that ships the tgz?

@NathanWalker
Copy link
Contributor Author

NathanWalker commented May 24, 2017

@sis0k0 I'll be back tomorrow afternoon and will take a look at that - should be able to rebase that outta there.

sis0k0 and others added 12 commits May 24, 2017 15:03
* chore(deps): update to Angular 4.1.0

* chore(lint): update tslint to ^5.1.0 and codelyzer to ^3.0.1

* refactor: fix lint errors

* fix: make update-app-ng-deps work with peerDeps
This should update all angular packages that the project depends on,
such as @angular/animations and @angular/compiler-cli.
Update emulator api level to 23
* fix(renderer): set templateParent for newly create DetachedText nodes

DetachedText is not inserted in the UI components tree, so we need to
add it's parent manually.

* fix(renderer): respect templateParent in parentNode() if one is set

fixes #777, fixes #787

* test: add unit test for NgIfElse and NgIfThenElse

* chore: target 'next' tag of tns-core-modules

* fix(renderer): stop attaching comments to visual tree

* refactor(renderer): create DetachedElements instead for comments and
text nodes

* refactor: move NgView, NgElement and similar to separate module
Change args for running nativescript-dev-appium
Update nativescirpt-dev-appium plugin to latest
Update --runtype=android23
…Async

That method is internal for Angular and available only within the JiT
compiler and we shouldn't expose it to user because it won't work within
AoT compiled apps.
sis0k0 and others added 4 commits May 24, 2017 15:03
NativeScriptRouterModule

SystemJsNgModuleLoader is injected in NSModuleFactoryLoader. That loader
can be used without router, so we need to provide the systemjs token in
the root NativeScriptModule instead.
we'll use e2e test for this functionality instead
@NathanWalker
Copy link
Contributor Author

@sis0k0 the commit that shipped the .tgz is now removed and latest master has been merged in.
Lemme know what else is needed here, thanks again for the help, excited for this to go in!

@sis0k0
Copy link
Contributor

sis0k0 commented May 25, 2017

uitests android

@sis0k0 sis0k0 merged commit 6a1f6a9 into NativeScript:master May 25, 2017
@vakrilov
Copy link
Contributor

... aaand it is merged! 🎉 🎉 🎉

@NathanWalker
Copy link
Contributor Author

Excellent thanks for all the help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants