fix(compiler-cli): add support for more than 2 levels of nested lazy routes #13678

Meligy · 2016-12-27T21:40:50Z

Please check if the PR fulfills these requirements

The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

What kind of change does this PR introduce? (check one with "x")

[x] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Other... Please describe:

What is the current behavior? (You can also link to an open issue here)

If you have more than 3 levels of nested lazy routes, like: {app-root}/lazy-loaded-module-1/lazy-loaded-module-2/lazy-loaded-module-3

Where lazy-loaded-module-3 is lazy loaded from lazy-loaded-module-2,
and lazy-loaded-module-2 is lazy loaded from module lazy-loaded-module-1,
and lazy-loaded-module-1 is lazy loaded from AppModule

You get only 2 chunk bundles created in addition to the main bundle, when you build your project using Angular CLI or @ng-tools Webpack plugin.

You'll be able to browse to {app-root}/lazy-loaded-module-1/lazy-loaded-module-2/ but get a router error in browser for {app-root}/lazy-loaded-module-1/lazy-loaded-module-2/lazy-loaded-module-3, because no bundle was generated for it.

What is the new behavior?

Your build will include the same number of chunks as your lazy modules, plus the main chunk of course.
Your browser will be able to go to any level of nested lazy-loaded modules you declared.

Does this PR introduce a breaking change? (check one with "x")

[ ] Yes
[x] No

~~If this PR contains a breaking change, please describe the impact and migration path for existing applications: ...~~

Other information:

This PR includes work from PR #13676, and superseeds it.

The PR can be tested on this sample repository https://github.com/meligy/routing-angular-cli
The test path in this sample is /lazy/deep/third/. The UI walks you level by level as well.

@hansl would be the best person to review this as he wrote the original implementation.

Fixes angular/angular-cli#3663

…oad routes paths The change avoids the compiler CLI internal API from mismatching the following case as lazy loading ``` import { NonLazyLoadedModule } from './non-lazy-loaded/non-lazy-loaded.module'; export function getNonLazyLoadedModule() { return NonLazyLoadedModule; } export const routes = [ { path: '/some-path', loadChildren: getNonLazyLoadedModule } ]; ``` The output of the check is later passed to `RouteDef.fromString()`, so, it makes sense to be only a string. Fixes angular/angular-cli#3204

…routes This change adds Compiler CLI support for any level of nesting for lazy routes. For example `{app-root}/lazy-loaded-module-1/lazy-loaded-module-2/lazy-loaded-module-3` Where `lazy-loaded-module-3` is lazy loaded from `lazy-loaded-module-2`, and `lazy-loaded-module-2` is lazy loaded from module `lazy-loaded-module-1`, and `lazy-loaded-module-1` is lazy loaded from `AppModule` Fixes angular/angular-cli#3663

Meligy · 2016-12-31T23:10:19Z

@hans @filipesilva this PR is ready for review now.

A few limitations that need to be addressed later:

Nested Routes `loadChildren` Paths Are Not Always Correct, But They Work As Long As They Are Unique

In the tests, we have the following tree:

Inside feature/lazy-feature.module.ts's routes, we got an entry like:

{path: 'feature', loadChildren: './feature.module#FeatureModule'}

When we generate the resultant RouteMap, if we set the map key to feature/feature.module#FeatureModule (so that it's root relative) instead of ./feature.module#FeatureModule (relative to feature/), the router fails in runtime, because it still looks the key ./feature.module#FeatureModule, because that's the value in loadChildren.

We can actually leave the incorrect path as the key (which the PR does now), and the router will match happily based on the absolute filename that we store separately in the map.

A side effect of that is that the following scenario will fail - Assume the following NgModule / Folder hierarchy:

app
- customers
  - details
- orders
  - details

If the details modules were not named order-details and customer-details, and they were both lazy loaded by relative path from customer and order, this will fail for the last discovered lazy route with a build time error that ./details/details.module#DetailsModule already exists in the map.

Solution

The right way to do this IMO is to rewrite the value of the loadChildren property to the right root-relative path, so that we use the same value in the RouteMap and the actual @NgModule() decorator. This needs to affect only generated JS result of course.

I don't know how to do that though. I'm assuming just setting the value we already read is not enough to persist it because it's just cached in an in-memory cache.

If we know how to do this, I'm happy to include it in this PR, if we don't, I make a case for this PR to be an improvement over the previous state (which had the same issue anyway, it's just more obvious the more levels you have).

`export default` NgModules Can Be Lazy Loaded From Other NgModules, But Cannot Lazy Load Other NgModules

While we can link to the file that has a default export in the resultant RouteMap, we cannot obtain a StaticSymbol from the file. Which means we cannot get the @NgModule() decorator or the routes inside it to analyze for nested loadChildren.

This is a compiler limitation not related to this API. I tried using default as class name and all. It just doesn't work. This PR does NOT introduce this limitation, it just caters for it.

I suggest for now the official word be we don't support default exports.

filipesilva · 2016-12-31T23:40:08Z

This is one for @hansl to review, he's the one doing the brunt of the work on lazy loading in the CLI.

conor-mac-aoidh · 2017-01-06T09:52:09Z

This should be merged asap! My app is broken due to this issue and the fix proposed by @Meligy is working! Thanks

hansl

LGTM

matsko · 2017-01-09T23:30:01Z

Merged as 5d9cbd7

Meligy · 2017-01-10T07:15:28Z

@hansl @matsko the merge commit is missing the first commit in this PR -- that's 93af424

Without it, non lazy loading of other modules is still broken. This PR is approved as both commits.

Please merge from this PR, or from the earlier PR #13676 which is just the missing commit.

matsko · 2017-01-10T08:16:06Z

@Meligy please ensure that all commits are in the same PR. The PR you linked has not been reviewed by anyone and therefore has not been set for merge.

Does this pose an issue that the commit in this PR has already been merged prior to the one in the other PR? Can it be merged later once it has been reviewed by someone on the core team?

Meligy · 2017-01-10T09:42:29Z

Hi @matsko,

As mentioned in the description above, this PR builds on top of (and includes) the work from the other PR.

The 2 commits can be applied in any order, and @hansl marked LGTM based on both commits (he asked me on the angular team slack to squash the commits, and accepted having them separate because they solve different -yet related- issues).

So, you may cherry pick the commit from this branch or the other PR, and apply it later. It should still work just fine.

Thanks a lot.

Meligy · 2017-01-10T10:18:20Z

Explanation:

First, I worked on 1 commit that is the other PR #13676.

Then I worked on this PR, and thought that because both issues are very related (but not dependent, only closely related), it might be easier to base my branch on the other PR and get a single approval -- this is what happened.

This means the branch of this PR has 2 commits, as shown in this PR, and they are approved together.

The PR could have been merged directly to master because it doesn't require an extra merge commit that would be undesirable in the commit log. Unfortunately what happened instead was cherry picking only one of the 2 commits in this PR.

So, you can simply ignore the other PR and just look at the missing approved commit in this PR. It's the same change anyway.

Sorry for the confusion, and I hope I have clarified things a bit better.

matsko · 2017-01-11T02:09:48Z

@Meligy I'm sorry for the mistake. This was 100% my fault. I had done a git cherry-pick on the branch instead of each commit within (I thought there was only one commit).

I've merged your code into master. Thank you for following up on this.

JSMike · 2017-01-11T15:26:07Z

@matsko if this was cherry picked to master will this be in the next 2.* release? or will we need to wait for 4.0?

DzmitryShylovich · 2017-01-11T15:27:14Z

fixes in 2.x
features in 4.0

matsko · 2017-01-11T15:29:22Z

It will show up in today's beta release of 4.0 and 2.4.3 (or tomorrow at the latest).

JSMike · 2017-01-11T15:32:11Z

Thank you :)

angular-automatic-lock-bot · 2019-09-10T14:21:54Z

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

googlebot added the cla: yes label Dec 27, 2016

Meligy force-pushed the fix/nested-lazy-routes branch 5 times, most recently from 4152054 to 3e680be Compare December 31, 2016 09:26

Meligy force-pushed the fix/nested-lazy-routes branch from 3e680be to b8158f0 Compare December 31, 2016 09:27

Meligy force-pushed the fix/nested-lazy-routes branch from b8158f0 to 9daed3e Compare December 31, 2016 22:41

vicb requested a review from hansl January 2, 2017 21:42

clydin mentioned this pull request Jan 6, 2017

ng build -aot error 'entry.split is not a function' on lazy loaded modules angular/angular-cli#3204

Closed

Meligy mentioned this pull request Jan 7, 2017

Nested lazy loading no longer works, bundles not being generated - beta 24 angular/angular-cli#3663

Closed

hansl approved these changes Jan 8, 2017

View reviewed changes

hansl added pr_state: LGTM action: merge The PR is ready for merge by the caretaker labels Jan 8, 2017

matsko closed this Jan 9, 2017

Meligy mentioned this pull request Jan 10, 2017

fix(compiler-cli): avoid handling functions in loadChildren as lazy load routes paths #13676

Closed

3 tasks

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 10, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(compiler-cli): add support for more than 2 levels of nested lazy routes #13678

fix(compiler-cli): add support for more than 2 levels of nested lazy routes #13678

Meligy commented Dec 27, 2016 •

edited

Loading

Meligy commented Dec 31, 2016 •

edited

Loading

filipesilva commented Dec 31, 2016

conor-mac-aoidh commented Jan 6, 2017

hansl left a comment

matsko commented Jan 9, 2017

Meligy commented Jan 10, 2017

matsko commented Jan 10, 2017

Meligy commented Jan 10, 2017

Meligy commented Jan 10, 2017

matsko commented Jan 11, 2017

JSMike commented Jan 11, 2017

DzmitryShylovich commented Jan 11, 2017

matsko commented Jan 11, 2017 •

edited

Loading

JSMike commented Jan 11, 2017

angular-automatic-lock-bot bot commented Sep 10, 2019

fix(compiler-cli): add support for more than 2 levels of nested lazy routes #13678

fix(compiler-cli): add support for more than 2 levels of nested lazy routes #13678

Conversation

Meligy commented Dec 27, 2016 • edited Loading

Meligy commented Dec 31, 2016 • edited Loading

Nested Routes loadChildren Paths Are Not Always Correct, But They Work As Long As They Are Unique

Solution

export default NgModules Can Be Lazy Loaded From Other NgModules, But Cannot Lazy Load Other NgModules

filipesilva commented Dec 31, 2016

conor-mac-aoidh commented Jan 6, 2017

hansl left a comment

Choose a reason for hiding this comment

matsko commented Jan 9, 2017

Meligy commented Jan 10, 2017

matsko commented Jan 10, 2017

Meligy commented Jan 10, 2017

Meligy commented Jan 10, 2017

Explanation:

matsko commented Jan 11, 2017

JSMike commented Jan 11, 2017

DzmitryShylovich commented Jan 11, 2017

matsko commented Jan 11, 2017 • edited Loading

JSMike commented Jan 11, 2017

angular-automatic-lock-bot bot commented Sep 10, 2019

Meligy commented Dec 27, 2016 •

edited

Loading

Meligy commented Dec 31, 2016 •

edited

Loading

Nested Routes `loadChildren` Paths Are Not Always Correct, But They Work As Long As They Are Unique

`export default` NgModules Can Be Lazy Loaded From Other NgModules, But Cannot Lazy Load Other NgModules

matsko commented Jan 11, 2017 •

edited

Loading