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

Cornerstone 2.0: Performance improvements #1229

Merged
merged 8 commits into from
May 18, 2018
Merged

Cornerstone 2.0: Performance improvements #1229

merged 8 commits into from
May 18, 2018

Conversation

mattolson
Copy link
Contributor

@mattolson mattolson commented May 8, 2018

What?

The goal of this release is to improve frontend theme performance. This is the work of @mattolson, @junedkazi and @BC-EChristensen during the Stranger Hackings hackathon.

Remove Modernizr

  • Modernizr is a blocking script that has very little benefit for us. Removing this script improves page responsiveness.
  • We used Modernizr via the css classes csscolumns, flexbox, objectfit, and js.
  • The csscolumns class was used for productMasonry for older browsers, but since then all of our supported browsers support css columns. There is one small problem with Firefox: it does not support break-inside, but it does support page-break-inside, which does the same thing. See https://caniuse.com/#feat=multicolumn
  • The flexbox class is unused
  • The objectfit class is used by carousel. Reimplmented without using Modernizer. The previous implmentation put a background image on the wrapper div and then hid it if object-fit is supported. The new implementation does not put the background image on the wrapper div by default, but instead assumes that your browser supports object-fit (all ours do except for IE). For IE, we use javascript to copy the image source from the image tag to the background image of the wrapper div and then hide the image.
  • The js class is used for a few things in css, so mimic this behavior with a simple inline script.

Save space for carousel

  • Add wrapper div for hero carousel that has the appropriate height for the image to be displayed after lazy loading.
  • This improves frontend performance because the browser will be less likely to load images below the fold immediately.

Add dns prefetch hints

  • Add dns prefetch & preconnect to core resources that we know we will need (configured cdn and font providers). This uses a new helper resourceHints that was introduced in paper 2.0.8.

Webpack 4 & Javascript optimizations

  • Upgrade to webpack 4
  • Clean up stencil bootstrap
  • Simplify interface for PageManager and get rid of async library dependency
  • Get rid of html5-history-api library dependency (no longer needed)
  • Get rid of fastclick library dependency (no longer needed)
  • Upgrade several babel dependencies
  • Separate webpack config into common, dev, prod, and add npm scripts to build
  • Get rid of minifications in babel loader, instead rely on webpack
  • Get rid of LoaderOptionsPlugin
  • Get rid of CommonsChunkPlugin (webpack 4 will automaticaly do this for prod)

Inject svg via ajax

  • Use svg-injector to fetch the svg icon sprite rather than embed it directly on the page. This improves frontend performance because the icons will be cached after the first request and we reduce the page weight by about 28kb.
  • Move location of generated svg so it is retrievable by the frontend through the CDN.
  • Update svgstore grunt task

Reduce defaults for product counts

  • Reduce the default number of featured, new, and bestselling products on the home page.
  • These can always be updated in Theme Editor, but the defaults don't need to be so high.

Lazier image loading

  • Use laziest possible setting for images

Screenshots

Using lighthouse for benchmarking against a production store, I found the following:

Before

Desktop

screen shot 2018-05-07 at 5 29 36 pm

Mobile

screen shot 2018-05-07 at 5 30 19 pm

After

Desktop

screen shot 2018-05-11 at 4 23 04 pm

Mobile

screen shot 2018-05-11 at 4 20 21 pm

@bigbot
Copy link

bigbot commented May 8, 2018

Autotagging @bigcommerce/storefront-team @davidchin

@bookernath
Copy link
Contributor

I'm not qualified to review this but I am totally stoked about it!

config.json Show resolved Hide resolved
@junedkazi
Copy link
Contributor

junedkazi commented May 8, 2018

Also just want to confirm that the screenshots are correct. The time stamp for the before is later than the after if it makes sense 😜

// Find the appropriate page loader based on pageType
const pageClassImporter = pageClasses[pageType];
if (typeof pageClassImporter === 'function') {
const PageClass = (await pageClassImporter()).default;
Copy link
Contributor

Choose a reason for hiding this comment

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

i can speak for scala world when i say "Awaits" are not great. Not sure if its the same in JS tho

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The async/await pattern is an alternate way of working with promises. await is essentially the same as Promise.then without all the code nesting. https://javascript.info/async-await

Copy link
Contributor

@icatalina icatalina May 11, 2018

Choose a reason for hiding this comment

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

Does await get transpiled by stencil-cli?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, await is non-blocking in JS, so not like the Await object in Scala. Fun fact, there's an async/await macro library for Scala that does the same thing: https://github.com/scala/scala-async

It basically rewrites the code to use .map and .flatMap in the compiler.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@icatalina Yes, we are running everything through babel in stencil CLI.

Copy link
Contributor

Choose a reason for hiding this comment

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

const pageClass = await xx <-- how can this be async what if somone uses the pageClass val before the await is resolved?

Copy link
Contributor

@tpietsch tpietsch May 17, 2018

Choose a reason for hiding this comment

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

RegisteredCustomerMessagesUITest::testRequiredFieldWhileCreatingMessage

same this.context undefined message

along with this updated (removed span)

    const REQUIRED_MESSAGE_XPATH    = '//textarea[@id = "message_content"]';
    const REQUIRED_SUBJECT_XPATH    = '//input[@id = "message_subject"]';

Copy link
Contributor

Choose a reason for hiding this comment

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

@tpietsch for the const pageClass = await xx example, it's simply sugar for using a promise.then, so it will not set the pageClass val until after the await is resolved. but it's non-blocking in that the thread is released when invoking await to handle other things.

Copy link
Contributor

@tpietsch tpietsch May 17, 2018

Choose a reason for hiding this comment

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

it just kinda not my fave cause now you have a variable in scope that you can use where the value might change right? first its undef then its def and between that time it can be read from incorrectly
In a normal Promise(resolve -> resolve.data) <-- you are more explicit about when the data is being used here it seems like it could cause intermittent weirdness. And also the variable is declared as a const which i would expect as non-changing value

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe Babel actually dumps a bunch of polyfill code in order to support async/await (it needs to support ES6 generator). Wouldn't this increase the file size, as you need both Promise and Generator instead of just Promise?

webpack.prod.js Outdated Show resolved Hide resolved
webpack.common.js Outdated Show resolved Hide resolved
@tpietsch
Copy link
Contributor

tpietsch commented May 8, 2018

noticed lots of VARS for the require dependecys that prob should be const right?

@mattolson
Copy link
Contributor Author

@junedkazi yes the screenshots are correct. I was switching back and forth between Cornerstone 1.17 and a custom theme based on this branch. I did Cornerstone 1.17 after my branch.

* Modernizr is a blocking script that has very little benefit for us. Removing this
  script improves page responsiveness.
* We used Modernizr via the css classes `csscolumns`, `flexbox`, `objectfit`, and `js`.
* The `csscolumns` class was used for productMasonry for older browsers, but since then
  all of our supported browsers support css columns. There is one small problem with
  Firefox: it does not support break-inside, but it does support page-break-inside,
  which does the same thing. See https://caniuse.com/#feat=multicolumn
* The `flexbox` class is unused
* The `objectfit` class is used by carousel. Reimplmented without using Modernizer.
  The previous implmentation put a background image on the wrapper div and then
  hid it if object-fit is supported. The new implementation does not put the background
  image on the wrapper div by default, but instead assumes that your browser supports
  object-fit (all ours do except for IE). For IE, we use javascript to copy the image
  source from the image tag to the background image of the wrapper div and then hide
  the image.
* The `js` class is used for a few things in css, so mimic this behavior with a simple
  inline script.
@danieldelcore
Copy link
Contributor

Glorious 👏 PR 👏 Description 👏

templates/layout/base.html Outdated Show resolved Hide resolved
// Find the appropriate page loader based on pageType
const pageClassImporter = pageClasses[pageType];
if (typeof pageClassImporter === 'function') {
const PageClass = (await pageClassImporter()).default;
Copy link
Contributor

@icatalina icatalina May 11, 2018

Choose a reason for hiding this comment

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

Does await get transpiled by stencil-cli?

christensenep and others added 2 commits May 11, 2018 16:47
* Add wrapper div for hero carousel that has the appropriate height
  for the image to be displayed after lazy loading.
* This improves frontend performance because the browser will be less
  likely to load images below the fold immediately.
* Add dns prefetch & preconnect to core resources that we
  know we will need -- fonts, cdn, jirafe, etc
@mattolson
Copy link
Contributor Author

@icatalina ♻️

@icatalina
Copy link
Contributor

icatalina commented May 13, 2018

hi @mattolson !

what about the await? #1229 (comment)

@mattolson
Copy link
Contributor Author

@icatalina I already replied inline there -- we run everything through Babel as part of Stencil CLI.

config.json Show resolved Hide resolved
throw new Error(err);
}
$(document).ready(() => {
page.onReady.bind(page)();
Copy link
Contributor

Choose a reason for hiding this comment

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

how can you be sure this order doesnt matter?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think you need to call bind because onReady is already bounded to page.

mattolson and others added 5 commits May 17, 2018 15:04
* Upgrade to webpack 4
* Clean up stencil bootstrap
* Simplify interface for PageManager and get rid of async library dependency
* Get rid of html5-history-api library dependency (no longer needed)
* Get rid of fastclick library dependency (no longer needed)
* Upgrade several babel dependencies
* Separate webpack config into common, dev, prod, and add npm scripts to build
* Get rid of minifications in babel loader, instead rely on webpack
* Get rid of LoaderOptionsPlugin
* Get rid of CommonsChunkPlugin (webpack 4 will automaticaly do this for prod)
* Use svg-injector to fetch the svg icon sprite rather than
  embed it directly on the page. This improves frontend performance
  because the icons will be cached after the first request and
  we reduce the page weight by about 28kb.
* Move location of generated svg so it is retrievable by the frontend
  through the CDN.
* Update svgstore grunt task
* Reduce the default number of featured, new, and bestselling
  products on the home page.
* These can always be updated in Theme Editor, but the defaults
  don't need to be so high.
* Use laziest possible setting for images
* Use `resourceHints` helper from paper 2.0.8, which looks up
  configured cdn and font providers.
config.json Show resolved Hide resolved
@Ubersmake
Copy link
Contributor

💚 Stencil regression suite has passed with the exception of those tests that hit the changed featured/new/popular product counts.

Copy link
Contributor

@davidchin davidchin left a comment

Choose a reason for hiding this comment

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

This is exciting. It seems the changes really make a huge difference. 👏 I have a few small comments for you. Could you please take a look when you get a chance? Thanks!

.travis.yml Show resolved Hide resolved
// Find the appropriate page loader based on pageType
const pageClassImporter = pageClasses[pageType];
if (typeof pageClassImporter === 'function') {
const PageClass = (await pageClassImporter()).default;
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe Babel actually dumps a bunch of polyfill code in order to support async/await (it needs to support ES6 generator). Wouldn't this increase the file size, as you need both Promise and Generator instead of just Promise?

throw new Error(err);
}
$(document).ready(() => {
page.onReady.bind(page)();
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think you need to call bind because onReady is already bounded to page.

assets/js/theme/common/carousel.js Show resolved Hide resolved

// Common configuration, with extensions in webpack.dev.js and webpack.prod.js.
module.exports = {
Copy link
Contributor

Choose a reason for hiding this comment

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

If file size is an issue, you might want to consider adding performance.maxEntrypointSize and set a limit on the maximum file size for every bundle. It would warn you if, in the future, you accidentally bundle a really large third party library.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

webpack.prod.js Show resolved Hide resolved
webpack.prod.js Show resolved Hide resolved
devtool: 'source-map',
plugins: [
new webpack.DefinePlugin({
'process.env.NODE_ENV': 'development',
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if this is needed for development. 🍹

webpack.common.js Show resolved Hide resolved
package.json Show resolved Hide resolved
@mattolson mattolson merged commit 3e7318d into bigcommerce:master May 18, 2018
@mattolson mattolson deleted the 2.0 branch May 18, 2018 01:31
@mattolson
Copy link
Contributor Author

@davidchin I'm sorry, I didn't see your review prior to merging. I will address these comments in a future PR. We intend to continue to invest in Cornerstone performance.

@davidchin
Copy link
Contributor

@mattolson no worries.

@PeteyRev
Copy link
Contributor

@mattolson Super awesome. Cant wait to test this newest version out on our next client

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.