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

Some loading optimizations #69

Merged
merged 7 commits into from Jun 2, 2017

Conversation

jeffbcross
Copy link
Contributor

I included some quick wins to improve the application's loading performance. Each commit has a detailed description of what was done, and why. Here's a quick overview:

  • Remove redundant CSS rules that were ending up in every component
  • Remove lazy loading of feed route
  • Remove some unneeded packages from production build
  • Use a smaller fetch polyfill
  • Reduce bundles to single bundle which is loaded with async attribute.

Paired with @mgechev on these yesterday.

I have a separate branch where I'm pre-rendering the app-shell with webpack and @angular/platform-server, and have some other ideas how to leverage pre-rendering at runtime as well, which I'll chat with you about.

… where needed

Previously, the _themes.scss file included both variables, and @includes, which would cause
redundant CSS rules to be placed into every generated CSS file. 

This commit moves variables declarations into a new file, _theme_variables.scss, which can
safely be included in components or any other SASS file without side effects.

This reduced the total size of all chunks from 830,306 bytes to 484,706 (42% decrease).
Since the feed routes are required for the first screen of the 
application, lazy loading the feed components will most likely add more 
latency to the time to bootstrap the home page. This commit moves the
feed components into the AppModule.
 * Remove the environment.dev.ts, which isn’t being used (as far as I 
   could tell).
 * Move dev-specific polyfills (reflect and zone) to dev environment 
   (environment.ts) to reduce size of prod bundle since reflect is only 
   needed in JIT mode.
 * Use minified zone in prod environment (environment.prod.ts)
 * Get rid of now-empty polyfills.ts (and remove its import from 
   main.ts)
This inlines webpack’s inline script, which provides the required 
functions for other webpack bundles. The script is so small that it’s 
easier to just include it in the index page.
This merges the vendor bundle and main bundle together, in order to
make it easier to load all of the initial assets via a single script
tag using the `async` attribute. Using the `async` attribute, helps
webkit unblock the rendering of the page as remote scripts are loaded &
executed. Justification for `async` vs `defer` in following commit 
message.

This does have one slight cache disadvantage, which is that the main 
bundle will likely change at a greater frequency than the vendor bundle,
but now the entire bundle will need to be invalidated with each update.
Given the low frequency at which the app is re-deployed, this isn’t a
high cost.
This commit adds the `async` attribute to the only remote script on the
page, so that the browser can continue parsing and rendering the
document without waiting on the script to load and execute. The `async`
attribute is preferable to `defer` because `defer` will block DOM
parsing and rendering in Safari, whereas `async` will not. Since
`async` does not guarantee script execution order like `defer`, scripts
that depend on each other must be bundled and loaded together.
@housseindjirdeh
Copy link
Owner

Can't thank you and @mgechev enough for this. You've spotted so many small quick wins throughout the entire app which is amazing 🙌 .

Also thank you for detailing every single commit with a nice description ☺️. I appreciate you taking the time to do that, it made it so much easier to understand what this PR included.

@housseindjirdeh housseindjirdeh merged commit 0d78708 into housseindjirdeh:master Jun 2, 2017
@mgechev mgechev deleted the optimizations1 branch June 2, 2017 20:59
@addyosmani
Copy link

Epic work, @jeffbcross! 🎉

@housseindjirdeh is this deployed on production now?

@MarkPieszak
Copy link

This looks fantastic guys! @jeffbcross 🥇
How is the TTI looking now?

🍾

@mgechev
Copy link

mgechev commented Jun 3, 2017

@addyosmani with @jeffbcross we were testing the application here.

With my local Lighthouse, I get decent results. There are still a few minor things which can tune the performance further.

screen shot 2017-06-02 at 20 31 58

@jeffbcross
Copy link
Contributor Author

Worth noting: last night we were testing with app shell pre-rendering, which I haven't included in this PR because I'm working on publishing a package to npm to abstract away most of the config boilerplate.

@housseindjirdeh
Copy link
Owner

@addyosmani it is deployed to production!

Lighthouse performance results are looking quite better on my end actually

screen shot 2017-06-03 at 12 08 01 am

Did a couple of WPT runs as well but unfortunately First Interactive results aren't being displayed through their linked Lighthouse results (might be a bug?). Will definitely do a couple more runs tomorrow however. @MarkPieszak

@addyosmani
Copy link

There were some bugs in there earlier today that we deployed a fix for. Let me know if it's still borked in the morning and we can chase.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants