Skip to content

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

Closed
Meligy wants to merge 1 commit into
angular:masterfrom
Meligy:fix/exports-misread-as-routes
Closed

fix(compiler-cli): avoid handling functions in loadChildren as lazy load routes paths#13676
Meligy wants to merge 1 commit into
angular:masterfrom
Meligy:fix/exports-misread-as-routes

Conversation

@Meligy

@Meligy Meligy commented Dec 27, 2016 •
edited
Loading

Copy link
Copy Markdown
Contributor

Please check if the PR fulfills these requirements

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 use an official tool that uses the Angular Compiler CLI private API, like Angular CLI (or @ng-tools Webpack plugin), and you have some route code like:

import { NonLazyLoadedModule } from './non-lazy-loaded/non-lazy-loaded.module';

export function getNonLazyLoadedModule() { return NonLazyLoadedModule; }

export const routes = [
  { path: '/some-path', loadChildren: getNonLazyLoadedModule }
];

Building this (for example ng build using Angular CLI beta.24) shows a build error:

ERROR in entry.split is not a function

And it also shows in the browser.

What is the new behavior?

No build error in console or browser in that case. Build just succeeds and browser functionality shows no error. Loading modules (lazy or not) works as expected.

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:

The core problem in the previous case is that the compiler CLI internal API treats the function getNonLazyLoadedModule() as a lazy loading string
(like './non-lazy-loaded/non-lazy-loaded.module#NonLazyLoadedModule').

It passes the output as the entry argument to RouteDef.fromString(entry: string), where the error entry.split is not a function happens.

@hansl would be an ideal reviewer for this, having created the private API.

Fixes angular/angular-cli#3204

@Meligy
Meligy force-pushed the fix/exports-misread-as-routes branch from 39fdd97 to 2ce4ad5 Compare December 27, 2016 13:53
clydin
clydin reviewed Dec 27, 2016
View reviewed changes
Comment thread modules/@angular/compiler-cli/src/ngtools_impl.ts Outdated

@clydin clydin Dec 27, 2016

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.

Keep first check to avoid empty strings?

@Meligy Meligy Dec 27, 2016

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.

Empty strings are errors. It's unintentional that they silently did nothing I believe.

Maybe could give a better error message, but that's a separate improvement I preferred to keep unrelated to this PR.

@Meligy Meligy mentioned this pull request Dec 27, 2016
Closed
2 of 3 tasks
@filipesilva filipesilva mentioned this pull request Dec 29, 2016
Closed
@Meligy
fix(compiler-cli): avoid handling functions in loadChildren as lazy l…
93af424 
…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
@Meligy
Meligy force-pushed the fix/exports-misread-as-routes branch from 2ce4ad5 to 93af424 Compare December 31, 2016 09:29
@Meligy

Meligy commented Jan 10, 2017

Copy link
Copy Markdown
Contributor Author

Merged as part of #13678

@Meligy Meligy closed this Jan 10, 2017
@Meligy

Meligy commented Jan 10, 2017

Copy link
Copy Markdown
Contributor Author

Actually. the commit wa not merged.

@Meligy Meligy reopened this Jan 10, 2017
@hansl

hansl commented Jan 10, 2017

Copy link
Copy Markdown
Contributor

LGTM.

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

matsko commented Jan 11, 2017

Copy link
Copy Markdown
Contributor

Merged as aeed737

@matsko matsko closed this Jan 11, 2017
@Meligy

Meligy commented Jan 11, 2017

Copy link
Copy Markdown
Contributor Author

@matsko thanks heaps man.

@angular-automatic-lock-bot

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

Copy link
Copy Markdown

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 10, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@clydin clydin clydin left review comments

Assignees

@hansl hansl

Labels

action: merge The PR is ready for merge by the caretaker cla: yes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

ng build -aot error 'entry.split is not a function' on lazy loaded modules

5 participants

@Meligy @hansl @matsko @clydin @googlebot