Skip to content

fix(compiler-cli): ngtools should accept useFactory (lazy routes) - #15334

Closed
fulls1z3 wants to merge 1 commit into
angular:masterfrom
fulls1z3:master
Closed

fix(compiler-cli): ngtools should accept useFactory (lazy routes)#15334
fulls1z3 wants to merge 1 commit into
angular:masterfrom
fulls1z3:master

Conversation

@fulls1z3

@fulls1z3 fulls1z3 commented Mar 21, 2017

Copy link
Copy Markdown

Add the required functionality to _collectRoutes function of ngtools_impl.ts, so that the ngtools respect provided factories with p.useFactory (as well as provided values with useValue).

Closes #15305

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)
Providing a factory method (useFactory) for the DI token ROUTES causes the discovery of lazy-routes to fail. Further details are documented on #15305.

What is the new behavior?
With this PR, it's possible to provide a factory method for the DI token ROUTES, and @ngtools/webpack accepts the values provided by the factory with useFactory (as well as provided values with useValue).

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:
For these changes, no tests and documents have been added or changed. Actually, there were already no specific tests and documents found to evaluate/document the current and new behavior.

Burak Tasci
fix(compiler-cli): ngtools should accept useFactory (lazy routes)
d22acc7 
Add the required functionality to `_collectRoutes` function of `ngtools_impl.ts`, so that the `ngtools` respect provided factories with `p.useFactory` (as well as provided values with `useValue`).

Closes #15305
@fulls1z3 fulls1z3 mentioned this pull request Mar 21, 2017
Open
@fulls1z3

fulls1z3 commented Mar 24, 2017

Copy link
Copy Markdown
Author

Hi @alexeagle, @chuckjaz, @IgorMinar, @mhevery, @tbosch; anybody will have a look on this PR in future?

@chuckjaz

chuckjaz commented Mar 24, 2017

Copy link
Copy Markdown
Contributor

@hansl Can you look at this change?

@tbosch

tbosch commented Mar 25, 2017

Copy link
Copy Markdown
Contributor

In general, when using useFactory, our static metadata collection (which also the compiler-cli uses), will not look inside of the function that is provided as the useFactory, as to do this we would need to instantiate the application. I.e. if angular-cli wants to do generate something based on the actual routes, useFactory will prevent this.

I might not have understood what you are trying to achieve...

@tbosch

tbosch commented Mar 25, 2017

Copy link
Copy Markdown
Contributor

Agree, @hansl would probably be best to understand the use case...

@fulls1z3

fulls1z3 commented Mar 25, 2017 •
edited
Loading

Copy link
Copy Markdown
Author

Hi @tbosch, if you look at the referenced issue #15305, you could have a better understanding about what this PR aims to solve.

I.e. if angular-cli wants to do generate something based on the actual routes, useFactory will prevent this.

The current situation already prevents use of useFactory. Unless useFactory is provided, it would not interfere the current workflow (angular-cli generates sthg based on the actual routes).

@hansl

hansl commented Mar 27, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor

We collect the routing information in order to know (in advance) the routes for static code splitting into separate bundles. This fix does not provide this as we don't analyze the content of the provider (we can't).

Also, in general, we should spend a fair amount of time designing and discussing this issue rather than jumping to a half-working solution that does only solve one of the symptoms. I do think @ocombe has something on his timeline to fix the issue above properly.

I will discuss the issue itself with the i18n team internally, but I'm very confident that this PR does not fix anything you're trying to fix.

@hansl hansl mentioned this pull request Mar 27, 2017
Closed
@fulls1z3

fulls1z3 commented Mar 27, 2017 •
edited
Loading

Copy link
Copy Markdown
Author

Hi @hansl, thanks for both your detailed explanation and advices. In this case, I'll keep an eye on further releases, for possible fixes.

@fulls1z3 fulls1z3 closed this Mar 28, 2017
@mjamin mjamin mentioned this pull request Jul 11, 2018
Closed
@liujingbreak liujingbreak mentioned this pull request Nov 14, 2018
Closed
@angular-automatic-lock-bot

angular-automatic-lock-bot Bot commented Sep 11, 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 11, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

area: router cla: yes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

compiler-cli: ngtools should accept useFactory (lazy routes)

6 participants

@fulls1z3 @chuckjaz @tbosch @hansl @ocombe @googlebot