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
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 7 additions & 7 deletions .travis.yml
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
language: node_js
node_js:
- 4
- 6
sudo: required
dist: trusty
install:
- bundle install
- npm install -g grunt-cli
- npm install -g bigcommerce/stencil-cli
- npm install
- bundle install
- npm install -g grunt-cli
- npm install -g bigcommerce/stencil-cli
mattolson marked this conversation as resolved.
Show resolved Hide resolved
- npm install
script:
- grunt check
- stencil bundle
- grunt check
- stencil bundle
4 changes: 2 additions & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -159,8 +159,8 @@ Icons are delivered via a single SVG sprite, which is embedded on the page in
`templates/layout/base.html`. It is generated via a grunt task `grunt svgstore`.

The task takes individual SVG files for each icon in `assets/icons` and bundles
them together, to be inlined on the top of the theme, inside a handlebars partial.
Each icon can then be called in a similar way to an inline image via:
them together, to be inlined on the top of the theme, via an ajax call managed
by svg-injector. Each icon can then be called in a similar way to an inline image via:

```
<svg><use xlink:href="#icon-svgFileName" /></svg>
Expand Down
1 change: 1 addition & 0 deletions assets/img/icon-sprite.svg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
30 changes: 7 additions & 23 deletions assets/js/app.js
Original file line number Diff line number Diff line change
Expand Up @@ -63,32 +63,16 @@ window.stencilBootstrap = function stencilBootstrap(pageType, contextJSON = null
return {
load() {
$(async () => {
let globalClass;
let pageClass;
let PageClass;

// Finds the appropriate class from the pageType.
const pageClassImporter = pageClasses[pageType];
if (typeof pageClassImporter === 'function') {
PageClass = (await pageClassImporter()).default;
}

// Load globals
if (loadGlobal) {
globalClass = new Global();
globalClass.context = context;
Global.load(context);
}

if (PageClass) {
pageClass = new PageClass(context);
pageClass.context = context;
}

if (globalClass) {
globalClass.load();
}

if (pageClass) {
pageClass.load();
// 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?

PageClass.load(context);
}
});
},
Expand Down
10 changes: 4 additions & 6 deletions assets/js/theme/account.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,14 @@ import { classifyForm, Validators, insertStateHiddenField } from './common/form-
import swal from 'sweetalert2';

export default class Account extends PageManager {
constructor() {
super();
constructor(context) {
super(context);

this.$state = $('[data-field-type="State"]');
this.$body = $('body');
}

loaded(next) {
onReady() {
const $editAccountForm = classifyForm('form[data-edit-account-form]');
const $addressForm = classifyForm('form[data-address-form]');
const $inboxForm = classifyForm('form[data-inbox-form]');
Expand All @@ -27,7 +27,7 @@ export default class Account extends PageManager {
this.passwordRequirements = this.context.passwordRequirements;
mattolson marked this conversation as resolved.
Show resolved Hide resolved

// Instantiates wish list JS
this.wishlist = new Wishlist();
Wishlist.load();

if ($editAccountForm.length) {
this.registerEditAccountValidation($editAccountForm);
Expand Down Expand Up @@ -67,8 +67,6 @@ export default class Account extends PageManager {
}

this.bindDeleteAddress();

next();
}

/**
Expand Down
9 changes: 3 additions & 6 deletions assets/js/theme/auth.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@ import forms from './common/models/forms';
import { classifyForm, Validators } from './common/form-utils';

export default class Auth extends PageManager {
constructor() {
super();
constructor(context) {
super(context);
this.formCreateSelector = 'form[data-create-account-form]';
}

Expand Down Expand Up @@ -167,9 +167,8 @@ export default class Auth extends PageManager {

/**
* Request is made in this function to the remote endpoint and pulls back the states for country.
* @param next
*/
loaded(next) {
onReady() {
const $createAccountForm = classifyForm(this.formCreateSelector);
const $loginForm = classifyForm('.login-form');
const $forgotPasswordForm = classifyForm('.forgot-password-form');
Expand All @@ -193,7 +192,5 @@ export default class Auth extends PageManager {
if ($createAccountForm.length) {
this.registerCreateAccountValidator($createAccountForm);
}

next();
}
}
2 changes: 1 addition & 1 deletion assets/js/theme/brand.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import $ from 'jquery';
import FacetedSearch from './common/faceted-search';

export default class Brand extends CatalogPage {
loaded() {
onReady() {
if ($('#facetedSearch').length > 0) {
this.initFacetedSearch();
} else {
Expand Down
4 changes: 1 addition & 3 deletions assets/js/theme/cart.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,16 +8,14 @@ import { defaultModal } from './global/modal';
import swal from 'sweetalert2';

export default class Cart extends PageManager {
loaded(next) {
onReady() {
this.$cartContent = $('[data-cart-content]');
this.$cartMessages = $('[data-cart-status]');
this.$cartTotals = $('[data-cart-totals]');
this.$overlay = $('[data-cart] .loadingOverlay')
.hide(); // TODO: temporary until roper pulls in his cart components

this.bindEvents();

next();
}

cartUpdate($target) {
Expand Down
2 changes: 1 addition & 1 deletion assets/js/theme/category.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import $ from 'jquery';
import FacetedSearch from './common/faceted-search';

export default class Category extends CatalogPage {
loaded() {
onReady() {
if ($('#facetedSearch').length > 0) {
this.initFacetedSearch();
} else {
Expand Down
12 changes: 12 additions & 0 deletions assets/js/theme/common/carousel.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,4 +7,16 @@ export default function () {
if ($carousel.length) {
$carousel.slick();
}

// Alternative image styling for IE, which doesn't support objectfit
if (typeof document.documentElement.style.objectFit === 'undefined') {
$('.heroCarousel-slide').each(() => {
const $container = $(this);
mattolson marked this conversation as resolved.
Show resolved Hide resolved
const imgUrl = $container.find('img').data('lazy');

if (imgUrl) {
$container.css('backgroundImage', `url(${imgUrl})`).addClass('compat-object-fit');
}
});
}
}
3 changes: 2 additions & 1 deletion assets/js/theme/common/product-details.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,8 @@ export default class ProductDetails {
this.imageGallery.init();
this.listenQuantityChange();
this.initRadioAttributes();
this.wishlist = new Wishlist().load();

Wishlist.load();

const $form = $('form[data-cart-item-add]', $scope);
const $productOptionsElement = $('[data-product-option-change]', $form);
Expand Down
2 changes: 1 addition & 1 deletion assets/js/theme/compare.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import $ from 'jquery';
import swal from 'sweetalert2';

export default class Compare extends PageManager {
loaded() {
onReady() {
const message = this.context.compareRemoveMessage;

$('body').on('click', '[data-comparison-remove]', event => {
Expand Down
2 changes: 1 addition & 1 deletion assets/js/theme/contact-us.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import $ from 'jquery';
import forms from './common/models/forms';

export default class ContactUs extends PageManager {
loaded() {
onReady() {
this.registerContactFormValidation();
}

Expand Down
3 changes: 1 addition & 2 deletions assets/js/theme/gift-certificate.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,7 @@ import { defaultModal } from './global/modal';

export default class GiftCertificate extends PageManager {
constructor(context) {
super();
this.context = context;
super(context);

const $certBalanceForm = $('#gift-certificate-balance');

Expand Down
22 changes: 8 additions & 14 deletions assets/js/theme/global.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import $ from 'jquery';
import './common/select-option-plugin';
import 'html5-history-api';
import PageManager from './page-manager';
import quickSearch from './global/quick-search';
import currencySelector from './global/currency-selector';
Expand All @@ -15,21 +14,16 @@ import maintenanceMode from './global/maintenanceMode';
import carousel from './common/carousel';
import 'lazysizes';
import loadingProgressBar from './global/loading-progress-bar';
import FastClick from 'fastclick';
import sweetAlert from './global/sweet-alert';

function fastClick(element) {
return new FastClick(element);
}
import svgInjector from './global/svg-injector';

export default class Global extends PageManager {
/**
* You can wrap the execution in this method with an asynchronous function map using the async library
* if your global modules need async callback handling.
* @param next
*/
loaded(next) {
fastClick(document.body);
onReady() {
// Only load visible elements until the onload event fires,
// after which preload nearby elements.
window.lazySizesConfig = window.lazySizesConfig || {};
window.lazySizesConfig.loadMode = 1;

quickSearch();
currencySelector();
foundation($(document));
Expand All @@ -43,6 +37,6 @@ export default class Global extends PageManager {
maintenanceMode(this.context.maintenanceMode);
loadingProgressBar();
sweetAlert();
next();
svgInjector();
}
}
5 changes: 5 additions & 0 deletions assets/js/theme/global/svg-injector.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import svgInjector from 'svg-injector';

export default function () {
svgInjector(document.querySelectorAll('svg[data-src]'));
}
29 changes: 8 additions & 21 deletions assets/js/theme/page-manager.js
Original file line number Diff line number Diff line change
@@ -1,35 +1,22 @@
import async from 'async';
import $ from 'jquery';

export default class PageManager {
constructor(context) {
this.context = context;
}

before(next) {
next();
}

loaded(next) {
next();
type() {
return this.constructor.name;
}

after(next) {
next();
onReady() {
}

type() {
return this.constructor.name;
}
static load(context) {
const page = new this(context);
mattolson marked this conversation as resolved.
Show resolved Hide resolved

load() {
async.series([
this.before.bind(this), // Executed first after constructor()
this.loaded.bind(this), // Main module logic
this.after.bind(this), // Clean up method that can be overridden for cleanup.
], (err) => {
if (err) {
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.

});
}
}
12 changes: 1 addition & 11 deletions assets/js/theme/product.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,18 +16,14 @@ export default class Product extends PageManager {
this.$reviewLink = $('[data-reveal-id="modal-review-form"]');
}

before(next) {
onReady() {
// Listen for foundation modal close events to sanitize URL after review.
$(document).on('close.fndtn.reveal', () => {
if (this.url.indexOf('#write_review') !== -1 && typeof window.history.replaceState === 'function') {
window.history.replaceState(null, document.title, window.location.pathname);
}
});

next();
}

loaded(next) {
let validator;

// Init collapsible
Expand All @@ -53,13 +49,7 @@ export default class Product extends PageManager {
return false;
});

next();
}

after(next) {
this.productReviewHandler();

next();
}

productReviewHandler() {
Expand Down
2 changes: 1 addition & 1 deletion assets/js/theme/search.js
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ export default class Search extends CatalogPage {
urlUtils.goToUrl(url);
}

loaded() {
onReady() {
const $searchForm = $('[data-advanced-search-form]');
const $categoryTreeContainer = $searchForm.find('[data-search-category-tree]');
const url = Url.parse(window.location.href, true);
Expand Down
11 changes: 3 additions & 8 deletions assets/js/theme/wishlist.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@ import { api } from '@bigcommerce/stencil-utils';
import { defaultModal } from './global/modal';

export default class WishList extends PageManager {
constructor() {
super();
constructor(context) {
super(context);

this.options = {
template: 'account/add-wishlist',
Expand Down Expand Up @@ -83,7 +83,7 @@ export default class WishList extends PageManager {
});
}

load() {
onReady() {
const $addWishListForm = $('.wishlist-form');

if ($addWishListForm.length) {
Expand All @@ -93,9 +93,4 @@ export default class WishList extends PageManager {
this.wishlistDeleteConfirm();
this.wishListHandler();
}

loaded(next) {
this.load();
next();
}
}
Loading