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

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

Closed
christopherthielen opened this issue Feb 8, 2017 · 14 comments
Assignees
Labels
feature Issue that requests a new feature P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent type: faq

Comments

@christopherthielen
Copy link

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
Copy link
Contributor

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?

@filipesilva filipesilva added type: discussion P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent feature Issue that requests a new feature labels Mar 9, 2017
@fulls1z3
Copy link

fulls1z3 commented Mar 20, 2017

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
Copy link
Contributor

hansl commented Mar 27, 2017

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 angular/angular#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
Copy link
Author

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
Copy link
Contributor

#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
Copy link

oparisy commented May 5, 2017

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
Copy link
Contributor

wardbell commented May 6, 2017

@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
Copy link

oparisy commented May 8, 2017

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
Copy link
Author

christopherthielen commented May 9, 2017

@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
Copy link
Contributor

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

@Dunos
Copy link

Dunos commented Oct 20, 2017

Any progress on this?

@filipesilva
Copy link
Contributor

Also wanted to mention @gkalpak's work in angular/angular#18428 for lazy loaded embedded components.

@hansl
Copy link
Contributor

hansl commented Jan 23, 2018

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

@angular-automatic-lock-bot
Copy link

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature Issue that requests a new feature P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent type: faq
Projects
None yet
Development

No branches or pull requests

7 participants