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

fix(@ngtools/webpack): emit lazy routes errors on rebuilds #12418

Merged
merged 2 commits into from
Oct 23, 2018
Merged

fix(@ngtools/webpack): emit lazy routes errors on rebuilds #12418

merged 2 commits into from
Oct 23, 2018

Conversation

alan-agius4
Copy link
Collaborator

@alan-agius4 alan-agius4 commented Oct 1, 2018

At the moment lazy route errors are only emitted in the initial builds because in following builds we are only processed lazy routes that are declared in the changed files.

At the moment, we cannot cache the previously resolved routes as there is no way to track in which file the lazy route was declared so that we can bust the lazy route if it was removed.

Closes #12236
Closes #12311

@alan-agius4 alan-agius4 added the target: major This PR is targeted for the next major release label Oct 1, 2018
@alan-agius4 alan-agius4 added target: patch This PR is targeted for the next patch release and removed target: major This PR is targeted for the next major release labels Oct 1, 2018
@clydin
Copy link
Member

clydin commented Oct 1, 2018

Can you measure rebuild performance with this change? The original reason for the AST method was performance concerns.

@alan-agius4
Copy link
Collaborator Author

alan-agius4 commented Oct 1, 2018

@clydin looks to be in the same range on a same app.

Now:

3466ms
3280ms
3079ms
3229ms
3219ms

Before

3249ms
3038ms
3363ms
3125ms
3150ms

@alan-agius4 alan-agius4 added target: major This PR is targeted for the next major release and removed target: patch This PR is targeted for the next patch release labels Oct 1, 2018
@clydin clydin removed the cla: yes label Oct 5, 2018
@googlebot
Copy link

☹️ Sorry, but only Googlers may change the label cla: yes.

@clydin
Copy link
Member

clydin commented Oct 5, 2018

Blocking this for 7.0 to limit risk in the RC period.

@filipesilva
Copy link
Contributor

@alan-agius4 does this PR also fix #12311?

@ngbot
Copy link

ngbot bot commented Oct 15, 2018

Hi @alan-agius4! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

@alexeagle
Copy link
Contributor

@alan-agius4 this is ready to merge but has a merge conflict, could you rebase please?

At the moment lazy route errors are only emitted in the initial builds because in following builds we are only processed lazy routes that are declared in the changed files.

At the moment, we cannot cache the previously resolved routes as there is no way to track in which file the lazy route was declared so that we can bust the lazy route if it was removed.

Closes #12236
@alan-agius4
Copy link
Collaborator Author

@alexeagle rebase done.
@filipesilva - looks like it, I added a test based on the steps that the users is doing f634be0

Copy link
Contributor

@filipesilva filipesilva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool then, I updated the PR description to also close the other issue.

@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 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
target: major This PR is targeted for the next major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inconsistent results from "ng build --watch" LoadChildren - lazy loaded routes and relative path problems
6 participants