angular / angular-cli

CLI tool for Angular
https://cli.angular.dev
MIT License
26.79k stars 11.98k forks source link

@ngtools/webpack feature request: support code splitting by third parties #4541

Closed christopherthielen closed 6 years ago

christopherthielen commented 7 years ago

This is a feature request.

I'd like to start discussion around @ngtools/webpack supporting code splitting by third party libs (including ui-router-ng2).

Currently the code splitting support in @ngtools/webpack has a soft dependency on @angular/router.

My understanding is that:

Goal

I think the goal should be to eliminate the dependency on @angular/router and provide a router agnostic mechanism for third parties to supply code splitting information.

Today, UI-Router supports code splitting using @ngtools/webpack by importing the ROUTES token from @angular/router, then providing the ui-router states as that DI token, i.e., { provide: ROUTES, useValue: module.states, multi: true }. However, this forces ui-router to have a dependency on @angular/router. It also forces ui-router to emulate the structure of @angular/router by adding a loadChildren property.

Proposal 1

Make the DI token configurable by @ngtools/webpack.

Do not reference the token from @angular/router in ngtools-impl.ts, but pass the token in. UI-Router users could configure this somehow to use the STATES token, for example.

This proposal is rather limited in its usefulness. Only one library could support lazy loading + code splitting in a given app.

Proposal 2

Move and/or rename the DI token out of @angular/router

Perhaps the DI token could be moved to @angular/compiler-cli, @angular/common, or @angular/core. This eliminates the need to depend on @angular/router. Additionally, move the token to its own ES6 module. Today, importing the ROUTES token from @angular/router brings in unrelated symbols from @angular/router into the ui-router bundle even using rollup.

This proposal still conceptually ties lazy loading to the @angular/router. All terminology and code analysis assumes that lazy loading code processes @angular/router routes. Third parties have to emulate the router's structure.

Proposal 3

Introduce a routing-agnostic token such as ANALYZE_FOR_LAZY_LOAD.

Either @angular/router or third parties should provide lazy load and code splitting information using this token. I propose instead of providing an array of specifically structured objects such as ROUTES or STATES, that only the lazy load information need be provided, i.e.:

{ 
  provide: ANALYZE_FOR_LAZY_LOAD,
   useValue: [ 
    './child1/child1.module#Child1Module', 
    './child2/child2.module#Child2Module' 
  ]
}

The @angular/router would then provide the module's lazy load information by doing something like:

const lazyRoutes = ROUTES.map(x => x.loadChildren).filter(identity);
const provider = { provide: ANALYZE_FOR_LAZY_LOAD, useValue: lazyRoutes, multi: true };

The knowledge about the lazy load declaration object (i.e., what the loadChildren property means on a route) is moved from @angular/compiler-cli to the library that owns that structure (i.e., the @angular/router).

Of these three proposals, this one is my favorite as it separates concerns nicely and provides a clear mechanism for lib authors to use.

filipesilva commented 7 years ago

Heya @christopherthielen, thank you for making such a detailed issue about this topic.

I haven't anything to contribute myself, but will loop the appropriate people in.

@hansl @robwormald @IgorMinar what do you think of these proposals? Is there anything else planned that would address this?

fulls1z3 commented 7 years ago

It would be really useful, if @ngtools/webpack can discover routes provided with useFactory as well as it does with useValue:

{ provide: ROUTES, useFactory: (provideRoutesSomehow), deps: [CustomRouterService], multi: true },
{ provide: ANALYZE_FOR_ENTRY_COMPONENTS, useValue: routes, multi: true }
hansl commented 7 years ago

We cannot use this. End of story. There's absolutely no way we can know what provideRoutesSomehow does or what it returns while building the software (we would need to run it / halting problem), and we need static routing if we want to do code splitting in webpack. So even though your solution in https://github.com/angular/angular/pull/15334 might do something, I'm very confident it does the wrong thing.

I will discuss the issue itself with the Angular i18n team internally and figure out the best way to move forward.

christopherthielen commented 7 years ago

This issue is still very important to me and ui-router users. With the new npm package structure in angular 4.x, ui-router users now have to bundle the entire @angular/router library simply to provide the ROUTES token (which enables ngtools/webpack support)

I haven't seen any public discussion about this issue. Has there been any internal discussion @hansl @robwormald @IgorMinar and if so, are you amenable to any of the above three proposals (expecially number 3)?

filipesilva commented 7 years ago

https://github.com/angular/angular-cli/issues/5981 has a similar issue:


Bug Report or Feature Request (mark with an x)


- [ bug report ]
ERROR Error: Uncaught (in promise): Error: Cannot find module './app/example/template/test-template/test-template.module'.

### Versions.
@angular/cli: 1.0.0
node: 6.9.4
os: darwin x64

### Repro steps.
when load dynamic module, using SystemJsNgModuleLoader.load(this.path), in browser inspect, got this error: "ERROR Error: Uncaught (in promise): Error: Cannot find module "'./app/example/template/test-template/test-template.module'
files path:
-src/app/example/template/test-template/test-template.module.ts
-src/app/example/example.component.ts

In example.component.ts, I tried to use this.path as below but all cannot work :
1. src/app/example/template/test-template/test-template.module
2. app/example/template/test-template/test-template.module
3. ./app/example/template/test-template/test-template.module
4. ./template/test-template/test-template.module

### Mention any other details that might be useful.
Does angular-cli allow us to load a module dynamically?

Thanks.
oparisy commented 7 years ago

I'm coming from #5981 since it was closed and merged with this issue.

I understand the angular-cli design choice of treeshaking/statically analysing as much code as possible.

But should that systematically forbid any way to load external modules (i.e. not available to angular-cli at build time)? Obviously we would not benefit from tree shaking for those modules.

wardbell commented 7 years ago

@christopherthielen Sympathetic with your goal and proposal #3 seems on the right track although I haven't thought about it deeply.

Perhaps unfairly, I want to hijack your efforts at solving this problem. Would you mind showing me how you used the Angular router infrastructure to lazy load one of your modules? I realize that you're goal is to be free of the Angular router dependence. For my purpose I don't require that freedom. I could get by with loading the RouterModule and hijacking its async module loading and the CLI's router-oriented code-splitting magic. And while I could figure this out myself, if you've already done it ... to steal is divine, yes? ;-)

oparisy commented 7 years ago

I have just stumbled upon the possibility to define externals in webpack configuration, which are a way to define a set of libraries which are not bundled but loaded at runtime (using CommonJS as an example).

This would definitely do the trick for my use case since regexp patterns can be used and I could definitely enforce some naming scheme for my "dynamic" modules. It seems I'd have to use another loader than SystemJS, but that's a fair price to pay.

Now, I understand arbitrary modification of the webpack configuration in angular-cli is a big no-no (#1656).

So is this externals mechanism exposed by angular-cli? How can I configure it?

christopherthielen commented 7 years ago

@wardbell there are a few concerns to consider:

1) lazy loading of an NgModule (supporting AoT and JIT) 2) placement of lazy loaded NgModule injector in the injector tree 3) code splitting by webpack + @ngtools/webpack 4) lazy module discovery and compilation by ngc

For module lazy loading, you need to: 1a) lazy load the sources (System.import('./lazymodule')) 1b) For JIT, get the NgModule and compile it 1c) For AoT, get the NgModuleFactory

Instantiate the lazy module as an NgModuleRef. 2a) Pass the parent injector when creating (ngModuleFactory.create(parentInjector)) Remember, we have a tree of injectors. When created, the NgModule will have a child injector associated with it.
2b) Now you have to use the child injector from the NgModule to inject services defined by the lazy module.
2c) You also have to use the child injector to access the lazy module's ComponentFactory when creating components from the lazy NgModule.

It's been a while since I wrote this code, but this is my vague recollection: Both @ngtools/webpack code splitting and also ngc lazy module analysis and compilation is done in the same way. 3a) Provide an array of objects using the ROUTES token (from @angular/router) 3b) Make sure each object in the array has a loadChildren function (if it should lazy load) 3c) In the loadChildren function, return a promise for the NgModule or NgModuleFactory, (or a string when using @ngtools/webpack loader's magic encantation)

This allows ngc to statically analyze the dependency tree, finding child modules via loadChildren and then statically analyzing the child module recursively.

filipesilva commented 7 years ago

Just wanted to mention that there is some further conversation and workarounds in this thread: https://github.com/angular/angular/issues/18093.

Dunos commented 7 years ago

Any progress on this?

filipesilva commented 7 years ago

Also wanted to mention @gkalpak's work in https://github.com/angular/angular/pull/18428 for lazy loaded embedded components.

hansl commented 6 years ago

Moving the discussion here to #9343 where we talk about the implementation.

angular-automatic-lock-bot[bot] commented 5 years ago

This issue has been automatically locked due to inactivity. Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.