fix(ivy): don't load factories when using Ivy - #28685

I'm not sure this is the best way to address lazy loaded Ivy modules.

This way makes it use this.loadAndCompile, which is just calling NgModuleFactoryR3 when in Ivy mode.

Other approaches include making a separate Ivy only SystemJsNgModuleLoader , or sending in an option in SystemJsNgModuleLoaderConfig.

@IgorMinar / @kara / @alxhub can you advise?

I think this change is not right.

You should either enable factory shim generation OR not use NgModuleFactoryLoader.

But nobody should disable (or not enable) shimming and still try to use NgModuleFactoryLoader.

The reasoning being that if you don't want shims, then you should just use import() to lazy load code and not use our custom loaders that increase the complexity of the code.

Projects that want to be able to switch back and forth between the legacy and ivy pipelines should enable shims and use NgModuleFactoryLoader until they permanently switch over to Ivy.

I think that the only other option we should entertain is that in ivy we would load the path and extract the factory from the NgModule loaded via this.loadFactory(path) - but I'm not sure if we can do that reliably.. maybe we can??? in that case one could switch back and forth between ivy and legacy without having to enable factory shimming....

I think that the only other option we should entertain is that in ivy we would load the path and extract the factory from the NgModule loaded via this.loadFactory(path)

Maybe that could be done. But it would also require tooling (like the CLI, or others) to understand that there is another difference in behaviour for the loadChildren/NgModuleFactoryLoader interaction.

Let's list the existing interactions for clarity though. I also tested all of these with the latest snapshots and without any of the changes in this PR.

Assuming the following loadChildren:

    RouterModule.forRoot([{
      path: 'lazy',
      loadChildren: './lazy.module#LazyModule',
    }]),

The VE ones are:

• without AOT, NgModuleFactoryLoader tries to load lazy.module.js (works now)
• with AOT, NgModuleFactoryLoader tries to load lazy.module.ngfactory.js (works now)

With Ivy and allowEmptyCodegenFiles: true it should be the same as above:

• without AOT, NgModuleFactoryLoader tries to load lazy.module.js
  • does not work now, loadFactory is used but there is no .ngfactory.js
• with AOT, NgModuleFactoryLoader tries to load lazy.module.ngfactory.js (works now)

With Ivy and allowEmptyCodegenFiles: false:

• if loadChildren: './lazy.module#LazyModule', is replaced with loadChildren: () => import('./lazy/lazy.module').then(m => m.LazyModule),, then NgModuleFactoryLoader is not used at all (works now).

All of these interactions work with no changes today, with the exception of jit+ivy+allowEmptyCodegenFiles: true (which should be fixed).

The novel one you are proposing is Ivy and allowEmptyCodegenFiles: false, with loadChildren: './lazy.module#LazyModule',:

• without AOT, NgModuleFactoryLoader tries to load lazy.module.js
  • does not work now, loadFactory is used but there is no .ngfactory.js
• with AOT, NgModuleFactoryLoader tries to load lazy.module.js
  • does not work now, loadFactory is used but there is no .ngfactory.js

I think the new case is possible to implement.

But supporting it implies there is still a magic import call in the angular/core codebase that must be special cased by tooling, which is used whenever loadChildren has a string argument.

Angular CLI currently special cases the System.import() call within NgModuleFactoryLoader by:

• calling a private API in compiler-cli that lists lazy routes
• assembling a map of lazy routes to file names
• informing the bundler that those file names can be used by the magic import()

All of this adds to the rebuild times and requires maintenance. Not supporting the loadChildren string format would thus make builds faster, require less maintenance on our code, and require no special casing on third party tooling. So it sounds like a better long-term strategy to not have it.

The spanner in this approach is that allowEmptyCodegenFiles: true is also being used to signal support for the loadChildren string format in Ivy. So while this mode exists, the magic import() must be supported.

Added tests for all these Ivy lazy load variations in angular/angular-cli#13774.

I am looking at ways to support the allowEmptyCodegenFiles variations in Ivy but think there's a fundamental problem with that approach: allowEmptyCodegenFiles is a compile-time option but using SystemJsNgModuleLoader is runtime behaviour, and I'm not sure there is a way to transfer that information over.

As far as I can tell SystemJsNgModuleLoader is always provided by @angular/router.

The injected Compiler instance that SystemJsNgModuleLoader uses is provided by @angular/core. In VE, it is overriden in @angular/platform-browser-dynamic, but not in Ivy. So Ivy always uses the @angular/core Compiler.

SystemJsNgModuleLoader checks what kind Compiler to determine whether it should try to load .ngfactory.js files. If it has the @angular/core one, it loads them, otherwise it doesn't.

In Ivy it has the same Compiler in JIT and AOT, so always tries to load .ngfactory.js.

In VE the what determined the existence of .ngfactory.js files was JIT vs AOT. The way that was communicated to the runtime Angular execution was through the used platform: platform-browser-dynamic meant JIT, and platform-browser meant AOT.

But in Ivy what determines the existence of factories is not just JIT vs AOT, but rather an additional AOT-only compile-time option: allowEmptyCodegenFiles: true. There is currently no way to communicate this option to the runtime, thus it cannot determine if .ngfactory.js files should be loaded.

This leads to a couple of choices:

If we want allowEmptyCodegenFiles: true to serve as a switch within Ivy to support loading .ngfactory.js files, then that information must be provided to the runtime.

If we want allowEmptyCodegenFiles: true to generate .ngfactory.js files but not load them, then in Ivy SystemJsNgModuleLoader should not be provided.

If we want to support the loadChildren string format in Ivy, a NgModuleFactoryLoader must be provided that contains an import statement. This import statement must be special-cased by tooling.

Another advantage of only supporting the loadChildren import() format in Ivy is that tooling TS configuration becomes simplified.

We can point the tsconfig files property at main.ts and TS will figure out that import() calls include those files as well. This removes the complicated include/exclude setups we have.

So much this. Looking at the flamechart on recompile in our project with ivy i'm so confused:

Isn't one of the ideas behind ivy that it compiles file2file and doesn't need the knowledge of it's surroundings? And lazy loading is a solved problem in webpack and very fast.

@sod I'm not really sure what you're trying to highlight in that flamechart. I don't see anything about lazy routes there.

yes!!

did you check that we have no more of these flags elsewhere in the repo? (unless the test specifically tests the shims?) I usually use git grep for such checks

I think this change is not right.

You should either enable factory shim generation OR not use NgModuleFactoryLoader.

But nobody should disable (or not enable) shimming and still try to use NgModuleFactoryLoader.

The reasoning being that if you don't want shims, then you should just use import() to lazy load code and not use our custom loaders that increase the complexity of the code.

Projects that want to be able to switch back and forth between the legacy and ivy pipelines should enable shims and use NgModuleFactoryLoader until they permanently switch over to Ivy.

I think that the only other option we should entertain is that in ivy we would load the path and extract the factory from the NgModule loaded via this.loadFactory(path) - but I'm not sure if we can do that reliably.. maybe we can??? in that case one could switch back and forth between ivy and legacy without having to enable factory shimming....

this is not right.. the loaded type should have been of type NgModuleFactory extracted out of the ES module and class loaded via test#NamedModule. This test proves that we are not extracting the factory but simply returning the loaded class instead and that's not right.

I'm not sure if the code here has changed since this comment, but the test looks correct to me. We're testing that load has created the NgModuleFactory wrapper, and that the moduleType (the original "class" that was dynamically loaded) is the one we expected it to wrap.