Skip to content

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

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.

Sign up for GitHub

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

Closed
filipesilva wants to merge 2 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1a31e55
fix(ivy): don't load factories when using Ivy
filipesilva Feb 18, 2019
4d37a06
test(docs-infra): ignore lazy loading examples in Ivy
filipesilva Apr 9, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions aio/tools/examples/run-example-e2e.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,11 @@ const IGNORED_EXAMPLES = [
const fixmeIvyExamples = [
// fixmeIvy('unknown') app fails at runtime due to missing external service (goog is undefined)
'i18n',
// Needs a Angular CLI synced with the Ivy loadChildren: string support after
// https://github.com/angular/angular/pull/28685
'lazy-loading-ngmodules',
'router',
'ngmodules',
];

if (argv.ivy) {
Expand Down
5 changes: 3 additions & 2 deletions packages/core/src/linker/system_js_ng_module_factory_loader.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@


import {Injectable, Optional} from '../di';
import {ivyEnabled} from '../ivy_switch';

import {Compiler} from './compiler';
import {NgModuleFactory} from './ng_module_factory';
Expand Down Expand Up @@ -56,8 +57,8 @@ export class SystemJsNgModuleLoader implements NgModuleFactoryLoader {
}

load(path: string): Promise<NgModuleFactory<any>> {
const offlineMode = this._compiler instanceof Compiler;
return offlineMode ? this.loadFactory(path) : this.loadAndCompile(path);
const legacyOfflineMode = !ivyEnabled && this._compiler instanceof Compiler;

@filipesilva filipesilva Feb 18, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@IgorMinar IgorMinar Feb 23, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@IgorMinar IgorMinar Feb 23, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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....

@filipesilva filipesilva Feb 25, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@filipesilva filipesilva Feb 26, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@filipesilva filipesilva Feb 26, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@filipesilva filipesilva Feb 28, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@sod sod Mar 5, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

image

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.

@filipesilva filipesilva Mar 19, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return legacyOfflineMode ? this.loadFactory(path) : this.loadAndCompile(path);
}

private loadAndCompile(path: string): Promise<NgModuleFactory<any>> {
Expand Down
31 changes: 27 additions & 4 deletions packages/core/test/linker/system_ng_module_factory_loader_spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import {Compiler, SystemJsNgModuleLoader} from '@angular/core';
import {global} from '@angular/core/src/util/global';
import {async} from '@angular/core/testing';
import {afterEach, beforeEach, describe, expect, it} from '@angular/core/testing/src/testing_internal';
import {modifiedInIvy, onlyInIvy} from '@angular/private/testing';

function mockSystem(modules: {[module: string]: any}) {
return {
Expand All @@ -20,9 +21,9 @@ function mockSystem(modules: {[module: string]: any}) {
};
}

{
describe('SystemJsNgModuleLoader', () => {
let oldSystem: any = null;
describe('SystemJsNgModuleLoader', () => {
let oldSystem: any = null;
modifiedInIvy('only loads ngfactory shims in View Engine').describe('(View Engine)', () => {
beforeEach(() => {
oldSystem = global['System'];
global['System'] = mockSystem({
Expand Down Expand Up @@ -54,4 +55,26 @@ function mockSystem(modules: {[module: string]: any}) {
});
}));
});
}

onlyInIvy('loads modules directly in Ivy').describe('(Ivy)', () => {
beforeEach(() => {
oldSystem = global['System'];
global['System'] = mockSystem({
'test': {'default': 'test module', 'NamedModule': 'test NamedModule'},
});
});
afterEach(() => { global['System'] = oldSystem; });

it('loads a default module', async(() => {
const loader = new SystemJsNgModuleLoader(new Compiler());
loader.load('test').then(
contents => { expect(contents.moduleType).toBe('test module' as any); });
}));
it('loads a named module', async(() => {
const loader = new SystemJsNgModuleLoader(new Compiler());
loader.load('test#NamedModule').then(contents => {
expect(contents.moduleType).toBe('test NamedModule' as any);

@IgorMinar IgorMinar Feb 23, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@alxhub alxhub Apr 12, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

});
}));
});
});