feat(aio): lazy-load embedded components #18428
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the |
0fcee25 to
b290e3e
Compare
|
Hi! I'm really sorry to creep on your PR but I'm really interested in your embedded component lazy load approach. I'm working on a library: https://github.com/josephliccini/ngx-lazy-view I like your approach, especially with the list of components, where mine right now is just one entry per module. Maybe we can work to build something standard for this pattern since I've seen this request often for this functionality. Is the mechanism you are using (similar to mine) going to be compatible with future versions of angular and the cli? We need to be able to lazy load components using this kind of pattern but don't want to resort to anything too nonstandard that could break between angular or cli releases.. Maybe we can move this over to an issue if this is the wrong forum. Thanks! |
b290e3e to
08d042e
Compare
|
CLAs look good, thanks! |
|
I haven't been able to figure out why, but the For example, below are the differences for the sourcemaps' main.bundle.js.map diff (
|
|
@josephliccini, I totally agree that lazy-loading modules/components should be more straightforward (and decoupled from the Router - see also angular/angular-cli#4541).
TBH, this approach is specific to the angular.io app. I doubt it is a good general design pattern. In any case, this is just a mechanism to detect when to load the lazy module. Each usecase will be different and (for a reusable library) it should be easy for people to plug their own logic.
I think there are still things to figure out before moving to a stable, reusable solution (but this is were we are heading 😃). The main issue at the moment (besides figuring out why the main bundle grew instead of shrinking 😁) is making it more straight forward to use the cli or
I am only using public, documented APIs, but some of them are experimental (so subject to change in future versions). cc @IgorMinar, @hansl |
56e5c5a to
6782866
Compare
|
You can preview 6782866 at https://pr18428-6782866.ngbuilds.io/. |
6782866 to
511ce07
Compare
|
You can preview 511ce07 at https://pr18428-511ce07.ngbuilds.io/. |
|
@gkalpak thanks for an excellent and thorough response! If the angular.io app is using this mechanism, I feel it may be safe in the short term to 'trick' the CLI as is is the only way for now. But I'm happy to work towards a future solution with other folks who are already working on this. If there's any way I can contribute towards this please let me know. Thanks! |
|
Let's fix and land this first (I am sure there are useful insights to be gained down the road 😃) |
|
Hey @gkalpak I have a hypothesis where your size gains are coming from. Is this branch you are using already using the If so, it may be that a code split point will incur a scope-hoisting break (described here: https://medium.com/webpack/webpack-freelancing-log-book-week-5-7-4764be3266f5) It seems that UglifyJS needs all the I might be wrong, but I think I am running into the same issue with code-splitting and build-optimizer Edit: adding @hansl who will know more than I do. |
|
I was working with @filipesilva in the gitter.im room, but probably better to have this discussion out so the knowledge and findings are shared, so I will repeat what I mentioned in there so all can see. When I went into my app, and hooked up After discussing, I turned off all code-splitting, and Angular dropped 200KB I believe the Furthermore, I think it explains why you needed to set Zone.js (and maybe other places in Angular?) do Again just my findings so far... We can move this elsewhere for discussion if needed. |
|
cc @IgorMinar who is currently investigating this. |
|
Heya @josephliccini, great job looking into this! Very good insight on your post. angular/devkit#89 would address this by making webpack itself drop these modules, but it's still a ways off. |
|
@filipesilva no prob. we are super interested in both lazy loading outside router (initial reason I commented on this PR) and Tree Shaking / Build Optimizer with Code Splits so we are happy to help in any way. Feel free to mail or ping on gitter any time. |
aio/src/app/app.module.ts
Outdated
| @@ -1,20 +1,19 @@ | |||
| import { BrowserModule } from '@angular/platform-browser'; | |||
| import { NgModule } from '@angular/core'; | |||
| import { InjectionToken, NgModule, NgModuleFactoryLoader, SystemJsNgModuleLoader } from '@angular/core'; | |||
There was a problem hiding this comment.
InjectionToken not used in this file
IgorMinar
added a commit
to IgorMinar/angular
that referenced
this pull request
Oct 17, 2017
This commit is based on @gkalpak's angular#18428.
IgorMinar
added a commit
to IgorMinar/angular
that referenced
this pull request
Oct 19, 2017
This commit is based on @gkalpak's angular#18428. -rw-r--r-- 1 iminar eng 84219 Oct 18 09:11 dist/0.8ef208c27531d5c6af63.chunk.js -rw-r--r-- 1 iminar eng 14942 Oct 18 09:11 dist/4.c719ac5645940382cdce.chunk.js -rw-r--r-- 1 iminar eng 1560 Oct 18 09:11 dist/inline.348e68003e86e3c91506.bundle.js -rw-r--r-- 1 iminar eng 492522 Oct 18 09:11 dist/main.5180cbac2700ee43b6f0.bundle.js -rw-r--r-- 1 iminar eng 37402 Oct 18 09:11 dist/polyfills.f8409a9eb69060ac1aa6.bundle.js
|
@gkalpak could you please resurrect this PR now that we are on v5? I'd love to get the size numbers for my AngularConnect presentation. |
a8c714b to
2d6ed50
Compare
|
You can preview 2d6ed50 at https://pr18428-2d6ed50.ngbuilds.io/. |
|
All fixed up. Applying the |
IgorMinar
approved these changes
Dec 6, 2017
There was a problem hiding this comment.
This looks good. As a followup items I suggest that we:
- reconfigure the cli to use named chunks so that it's easier to understand the lazy loaded filenames
- start tracking the size of all chunks
- reconfigure firebase to http2 push the lazy chunk otherwise we will make the full rendering time actually slower.
jasonaden
pushed a commit
that referenced
this pull request
Dec 8, 2017
#18428) Previously, the document was shown as soon as the HTML was received, but before the embedded components were ready (e.g. downloaded and instantiated). This caused FOUC (Flash Of Uninstantiated Components). This commit fixes it by preparing the new document in an off-DOM node and swapping the nodes when the embedded components are ready. PR Close #18428
jasonaden
pushed a commit
that referenced
this pull request
Dec 8, 2017
jasonaden
pushed a commit
that referenced
this pull request
Dec 8, 2017
… scroll timing (#18428) - Fix embedded ToC: Previously, the element was added too late and was never instantiated. - Improve ToC update timing: Previously, the ToC was updated after the entering animation was over, which resulted in the ToC being outdated for the duration of the animation. - Improve destroying components timing: Previously, the old embedded components were destroyed as soon as a new document was requested. Even if the transition ended up never happening (e.g. due to error while preparing the new document), the embedded components would have been destroyed and the displayed document would not work as expected. Now the old embedded components are destroyed only after the new document has been fully prepared. - Improve scroll-to-top timing: Previously, the page was scrolled to top after the entering animation was over, which resulted in "jumpi-ness". Now the scrolling happens after the leaving document has been removed and before the entering document has been inserted. PR Close #18428
jasonaden
pushed a commit
that referenced
this pull request
Dec 8, 2017
jasonaden
pushed a commit
that referenced
this pull request
Dec 8, 2017
jasonaden
pushed a commit
that referenced
this pull request
Dec 8, 2017
jasonaden
pushed a commit
that referenced
this pull request
Dec 8, 2017
jasonaden
pushed a commit
that referenced
this pull request
Dec 8, 2017
jasonaden
pushed a commit
that referenced
this pull request
Dec 8, 2017
#18428) Previously, the document was shown as soon as the HTML was received, but before the embedded components were ready (e.g. downloaded and instantiated). This caused FOUC (Flash Of Uninstantiated Components). This commit fixes it by preparing the new document in an off-DOM node and swapping the nodes when the embedded components are ready. PR Close #18428
jasonaden
pushed a commit
that referenced
this pull request
Dec 8, 2017
jasonaden
pushed a commit
that referenced
this pull request
Dec 8, 2017
… scroll timing (#18428) - Fix embedded ToC: Previously, the element was added too late and was never instantiated. - Improve ToC update timing: Previously, the ToC was updated after the entering animation was over, which resulted in the ToC being outdated for the duration of the animation. - Improve destroying components timing: Previously, the old embedded components were destroyed as soon as a new document was requested. Even if the transition ended up never happening (e.g. due to error while preparing the new document), the embedded components would have been destroyed and the displayed document would not work as expected. Now the old embedded components are destroyed only after the new document has been fully prepared. - Improve scroll-to-top timing: Previously, the page was scrolled to top after the entering animation was over, which resulted in "jumpi-ness". Now the scrolling happens after the leaving document has been removed and before the entering document has been inserted. PR Close #18428
jasonaden
pushed a commit
that referenced
this pull request
Dec 8, 2017
jasonaden
pushed a commit
that referenced
this pull request
Dec 8, 2017
jasonaden
pushed a commit
that referenced
this pull request
Dec 8, 2017
jasonaden
pushed a commit
that referenced
this pull request
Dec 8, 2017
4 tasks
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a first rough version (but functional nonetheless 😃).
Shout-out to @filipesilva for useful pointers and insights.
TODO:
Fixes #15629 and #16127.