Adopt handlebars precompiled templates #1752

sendilkumarn · 2017-02-18T15:58:39Z

@paulirish this will need us to run watch even when we are testing locally on cli, in order to compile the templated. Otherwise we have to split the generateReport into 2 different methods one handles with html files and other with precompiled template file.

Note: This is my first attempt with precompile ;)

paulirish · 2017-02-22T02:45:48Z

Hmm! So it looks like after this PR, a few of the compiled files actually get larger.

viewer/main.js goes from 320kb to 344kb
lighthouse-background.js goes from 2.5mb to 2.6mb

(i checked gzip sizes and they get a tiny bit bigger as well: 520k to 525k and 92.5k to 96.4k)

so it's not a win in filesize, at least. It might still be worth it for either runtime or complexity but i'm not sure about that yet.

regardless, i really appreciate you giving this one a shot, @sendilkumarn! super interesting.

sendilkumarn · 2017-02-22T05:29:46Z

@paulirish
Sorry.
Apparently, I did not include partials. So after including partials and then removing html references. This is what I ended up with. ( completely dereferencing handlebar to handlebar/runtime )

viewer/main.js goes from 320kb to 283kb
lighthouse-background.js goes from 2,541,212 kb to 2,452,002kb

paulirish

Nice work. Yeah the runtime require is key. :)

Happy this is in good shape.

WDYT, @ebidel? Shall we move ahead with this?

paulirish · 2017-02-23T03:23:33Z

lighthouse-extension/gulpfile.js

@@ -178,6 +207,14 @@ gulp.task('watch', ['lint', 'browserify', 'html'], () => {
    'app/src/**/*.js',
    '../lighthouse-core/**/*.js'
  ], ['browserify']);
+
+  gulp.watch([
+    '../lighthouse-core/**/*.html'


this and the partials wildcard should overlap, right? does the order of these compile tasks matter?

Yes. If we want to change anything, straightway in that precompiled file, this will help.

Order, yeah we have to run compile first and then browserify.

So they can't be combined? Also can we utilize config.report and config.partials for the globs?

paulirish · 2017-02-23T03:24:52Z

lighthouse-core/report/templates/report-templates.js

@@ -0,0 +1,283 @@
+this["report"] = this["report"] || {};


wondering if we should put this in gitignore instead.

That is debatable. If we add in gitignore, we need to make sure that it will be generated on the fly for people who just clones the repo and runs node-cli command.

ebidel · 2017-02-23T04:50:26Z

I like anything that makes our build smaller :)

But I'm not sure we should have the compile steps as part of the extension/viewer. For one, it's duplicated code. It would be nice to have shared tasks in a gulp_scripts dir in the top level repo. The other issue is that it might be time for a main gulpfile.js in lighthouse/? The compilation should really live above the viewer/extension.

sendilkumarn · 2017-02-23T04:55:05Z

Yeah that would be awesome. It will also solves the above comment
wondering if we should put this in gitignore instead.

patrickhulce

This is awesome stuff thanks @sendilkumarn!

Really unfortunate we're maintaining so many separate gulpfiles though :/ Also not a huge fan of introducing another forgettable build step when developing the CLI locally, especially a non-obvious one that requires you to run the watch of the extension to put files back into core. If we move forward with it, I agree with @ebidel on moving it up to a top-level gulpfile.

patrickhulce · 2017-02-23T17:52:03Z

lighthouse-core/report/report-generator.js

@@ -164,7 +150,11 @@ class ReportGenerator {
        Handlebars.registerHelper(helpers);
      }

-      Handlebars.registerPartial(audit.name, formatter.getFormatter('html'));
+      const partials =reportPartials.report.partials;


nit: space after =

wondering why eslint did not complain ...

patrickhulce · 2017-02-23T17:52:14Z

lighthouse-core/report/report-generator.js

@@ -133,7 +119,7 @@ class ReportGenerator {
   * @return {string} HTML of the exception page.
   */
  renderException(err, results) {
-    const template = Handlebars.compile(this.getExceptionTemplate());
+    const template = reportTemplate.report.template['exception'];


nit: .exception

patrickhulce · 2017-02-23T17:52:48Z

lighthouse-core/report/report-generator.js

+      const partials =reportPartials.report.partials;
+      Handlebars.registerPartial(audit.name,
+        Handlebars.template(
+          partials[audit.name] ? partials[audit.name] : partials['empty-formatter']


nit: partials[audit.name] || partials['empty-formatter']

sendilkumarn · 2017-02-24T01:39:08Z

Thanks @patrickhulce 👍

sendilkumarn · 2017-02-27T16:13:15Z

Added a gulp folder to write the common tasks and refer common tasks from there.

ebidel

Nice drying up the gulpfiles :)

Up until now, we haven't checked in built files. Personally, I don't think we should start going down that road to be consistent with other places in the codebase.

ebidel · 2017-02-27T19:17:27Z

gulp/compile-partials.js

@@ -0,0 +1,19 @@
+'use strict';


Can you drop our license header in these new files?

ebidel · 2017-02-27T19:22:53Z

lighthouse-core/test/formatter/formatters-test.js

@@ -43,6 +42,7 @@ const walkTree = new Promise((resolve, reject) => {
 });

 describe('Formatters', () => {
+  /* TODO : Remove these tests and add appropriate tests based on handlebar pre-compiled templates
  it('has no formatters failing when getFormatter("html") is called', () => {


Let's use .skip instead:

lighthouse/lighthouse-core/test/audits/screenshots-test.js

Line 32 in 6943b99

it.skip('processes an empty trace for screenshot data', () => {

ebidel · 2017-02-27T19:32:17Z

lighthouse-extension/gulpfile.js

@@ -178,6 +207,14 @@ gulp.task('watch', ['lint', 'browserify', 'html'], () => {
    'app/src/**/*.js',
    '../lighthouse-core/**/*.js'
  ], ['browserify']);
+
+  gulp.watch([
+    '../lighthouse-core/**/*.html'


So they can't be combined? Also can we utilize config.report and config.partials for the globs?

ebidel · 2017-02-27T19:32:25Z

lighthouse-viewer/gulpfile.js

-  ], ['browserify']);
-});
+    gulp.watch([
+      '../lighthouse-core/**/*.html'


same comment here.

ebidel

Do you know why the null-formatter.html is being included in files changed?

Do we also need to update https://github.com/GoogleChrome/lighthouse/blob/master/.travis.yml to run gulp? I would change https://github.com/GoogleChrome/lighthouse/blob/master/.travis.yml#L22-L24 to npm run build-all and add your gulp to the build-all script: https://github.com/GoogleChrome/lighthouse/blob/master/package.json#L18

ebidel · 2017-03-01T18:27:30Z

gulp/compile-report.js

+ * Copyright 2017 Google Inc. All rights reserved.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.


what do you think about combining these two files into one and exporting both functions? They're very similar.

changes to templates

sendilkumarn · 2017-03-02T01:27:54Z

@ebidel null-formatter is used for partials with no partial-template right ? am I wrong ?

sendilkumarn · 2017-03-02T01:28:43Z

👍 for travis I think that will be enough.

ebidel · 2017-03-02T02:43:43Z

Right. But it has 0 LOC changed:

Never seen that before. Not sure why that file is being touched.

sendilkumarn · 2017-03-02T03:03:16Z

Well it is just an empty file.

I created it. Before that null-formatter js will just return an empty string. Since we cannot use that for pre-compiling templates. Added that file

ebidel · 2017-03-02T06:00:45Z

Looks good @sendilkumarn! Did you see my comments on .travis.yml and package.json updates? #1752 (review)

sendilkumarn · 2017-03-02T06:07:59Z

Yeah absolutely fine with that.
Check here

I will add and push.

ebidel · 2017-03-02T07:44:50Z

package.json

@@ -15,7 +15,7 @@
    "install-cli": "cd ./lighthouse-cli && npm install",
    "install-extension": "cd ./lighthouse-extension && npm install",
    "install-viewer": "cd ./lighthouse-viewer && npm install",
-    "build-all": "npm run build-cli && npm run build-extension && npm run build-viewer",
+    "build-all": "npm run build-cli && npm run build-extension && npm run build-viewer && gulp",


Shouldn't gulp be first so the templates are compiled by the time other places need them?

changed the order of build running gulp at first

sendilkumarn · 2017-03-02T08:03:20Z

@ebidel yeah exactly. Thanks 👍

ebidel

LGTM from me. I'll keep open if anyone else has further comments.

patrickhulce

thanks for all this work!

patrickhulce · 2017-03-02T18:24:14Z

package.json

@@ -24,7 +24,7 @@
    "coverage": "node $(npm bin)/istanbul cover -x \"**/third_party/**\" _mocha -- $(find */test -name '*-test.js') --timeout 10000 --reporter progress",
    "coveralls": "npm run coverage && cat ./coverage/lcov.info | coveralls",
    "start": "node ./lighthouse-cli/index.js",


do we want to add gulp to start too since it won't be able to run without it now?

I'm leaning towards not incurring that cost every start, but a note in the README or something would be nice :)

do we need to have pre-compiled templates for global installs ? currently we are ignoring those.
I added gulp as an extra step in develop section. [Edited]

paulirish · 2017-03-04T00:56:22Z

Huge. Thanks @sendilkumarn for taking this on. This was a yeoman's work and we're really jazzed about it.

sendilkumarn · 2017-03-05T06:22:56Z

Thank you 👍

Adopt handlebars precompiled templates

b7d87a7

sendilkumarn closed this Feb 19, 2017

sendilkumarn reopened this Feb 19, 2017

Included gulp-concat / declare in dev dependencies

5c0270a

adding pre-compiled partials

02de109

paulirish reviewed Feb 23, 2017

View reviewed changes

patrickhulce reviewed Feb 23, 2017

View reviewed changes

nit fixes

ef58993

gulp : refactoring

40499b5

ebidel suggested changes Feb 27, 2017

View reviewed changes

sendilkumarn added 5 commits February 28, 2017 07:30

gulp config refactoring

b14f7cc

make location consistent

db7c3ed

replace empty to null formatter

9edb7ce

Added gulp to run before executing unit tests

b30759c

changed tests to skip

7520318

ebidel suggested changes Mar 1, 2017

View reviewed changes

merged common tasks in gulp

6ffcda4

changes to templates

sendilkumarn force-pushed the pre-compile branch from 033b20d to e50c840 Compare March 2, 2017 07:35

sendilkumarn closed this Mar 2, 2017

sendilkumarn reopened this Mar 2, 2017

ebidel reviewed Mar 2, 2017

View reviewed changes

travis build changes

0b4e544

changed the order of build running gulp at first

sendilkumarn force-pushed the pre-compile branch from e50c840 to 0b4e544 Compare March 2, 2017 07:54

changes to appveyor build

18a59c0

ebidel approved these changes Mar 2, 2017

View reviewed changes

patrickhulce approved these changes Mar 2, 2017

View reviewed changes

added note

d99a950

paulirish merged commit 3649322 into GoogleChrome:master Mar 4, 2017

patrickhulce mentioned this pull request Mar 6, 2017

Report: template is missing table/card/urllist formatters #1820

Closed

This was referenced Mar 6, 2017

Reevaluate stronger CSP for extension; remove unsafe-eval #1823

Closed

Simplify report generation: no pretty print, no formatter classes #1838

Merged

paulirish mentioned this pull request Mar 14, 2017

upload yarn.lock after precompiled templates PR #1862

Merged

brendankenny mentioned this pull request May 23, 2017

Adopt handlebars precompiled templates #1448

Closed

Adopt handlebars precompiled templates #1752

Adopt handlebars precompiled templates #1752

Conversation

sendilkumarn commented Feb 18, 2017

paulirish commented Feb 22, 2017 • edited

sendilkumarn commented Feb 22, 2017 • edited

paulirish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebidel commented Feb 23, 2017 • edited

sendilkumarn commented Feb 23, 2017

patrickhulce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sendilkumarn commented Feb 24, 2017

sendilkumarn commented Feb 27, 2017 • edited

ebidel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebidel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sendilkumarn commented Mar 2, 2017 • edited

sendilkumarn commented Mar 2, 2017

ebidel commented Mar 2, 2017

sendilkumarn commented Mar 2, 2017

ebidel commented Mar 2, 2017

sendilkumarn commented Mar 2, 2017

Choose a reason for hiding this comment

sendilkumarn commented Mar 2, 2017

ebidel left a comment

Choose a reason for hiding this comment

patrickhulce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sendilkumarn Mar 3, 2017 • edited

Choose a reason for hiding this comment

paulirish commented Mar 4, 2017

sendilkumarn commented Mar 5, 2017

paulirish commented Feb 22, 2017 •

edited

sendilkumarn commented Feb 22, 2017 •

edited

ebidel commented Feb 23, 2017 •

edited

sendilkumarn commented Feb 27, 2017 •

edited

sendilkumarn commented Mar 2, 2017 •

edited

sendilkumarn Mar 3, 2017 •

edited