add gulp recipe (wip) #1886

denar90 · 2017-03-21T22:25:22Z

Actual implementation is done, but I faced same issue

events.js:161
      throw er; // Unhandled 'error' event
      ^

Error: connect ECONNREFUSED 127.0.0.1:9222

as on pwmetrics.

Here some debug of this issue if someone is interested - paulirish/pwmetrics#63 (comment)

So, after solving this one it will be ready to merge.

P.S. Maybe @addyosmani will be interested to make webpack recipe :)

denar90 · 2017-03-22T06:29:55Z

Just realize that, before, chrome launcher has to be started. It's running in cli stuff. Is it worth to have recipes and stuff here or just in scope of pwmetrics?
cc @paulirish

addyosmani · 2017-03-24T18:27:05Z

I'd be interested to understand the scope of interest in build-tool integration docs :) I need to rev https://github.com/addyosmani/webpack-lighthouse-plugin to latest if we want to reference elsewhere in docs.

denar90 · 2017-03-24T20:51:05Z

It's really nice to have good recipes/build-tool integration docs for people who wanna implement Lighthouse for CI. For some of them it's critical to have implementation example...

ebidel · 2017-03-25T01:46:44Z

docs/recipes/gulp/gulpfile.js

+
+function handleOk() {
+  connect.serverClose();
+  process.exit(0);


I wonder if we can add something here to check the score and pass/fail based on that? This will always exit cleanly if running LH succeeds.

I thought that all LH log will be shown in a console.
E.g. in pwmetrics for pass/fail we use smth like smokehouse.
What would you suggest?
Maybe reviewer (if we talking about project where LH will be injected) will look at CI and say it has bad trace, but 😒 , who does that..., so I dunno...
But I think bringing smokehouse as user feature will be kinda overhead...

I was thinking from the perspective of a user that wants to "fail the build" if their LH score drops to something too low or if they want to use the score in some way. Maybe just add the results object param to handleOk(results) and stick a // TODO: use lighthouse results after connect.serverClose();?

ebidel · 2017-03-25T01:48:02Z

docs/recipes/gulp/gulpfile.js

+  return connect.server({
+    root: '../public',
+    livereload: true,
+    port: port


We tend to just use the key if the var name is the same:

livereload: true, port }

ebidel · 2017-03-25T01:48:44Z

docs/recipes/gulp/gulpfile.js

+const connect = require('gulp-connect');
+const lighthouse = require('../../../lighthouse-core');
+const perfConfig = require('../../../lighthouse-core/config/perf.json');
+const port = 8080;


const PORT

ebidel · 2017-03-25T01:51:10Z

docs/recipes/gulp/gulpfile.js

+
+const gulp = require('gulp');
+const connect = require('gulp-connect');
+const lighthouse = require('../../../lighthouse-core');


If the idea is that we have cut/paste examples, users of this example will write this:

const lighthouse = require('lighthouse'); const perfConfig = require('lighthouse/lighthouse-core/config/perf.json');

ebidel · 2017-03-25T01:54:46Z

package.json

@@ -34,7 +34,8 @@
    "chrome": "node ./lighthouse-cli/manual-chrome-launcher.js",
    "fast": "npm run start -- --disable-device-emulation --disable-cpu-throttling --disable-network-throttling",
    "smokehouse": "node lighthouse-cli/test/smokehouse/smokehouse.js",
-    "deploy-viewer": "cd lighthouse-viewer && gulp deploy"
+    "deploy-viewer": "cd lighthouse-viewer && gulp deploy",
+    "gulp-example": "gulp --gulpfile docs/recipes/gulp/gulpfile.js --cwd docs/recipes/gulp"


IMO, we leave this as a documented example and not add it to core. Instead, you can create a directory for this recipe (docs/recipes/gulp-example) and stick a new package.json in there. Having everything in a single directory will also be future proof. I suspect we'll have a bunch more recipes in the future. Having a folder for each would be good.

add pkg in recipes add install recipes script

ebidel

Good changes. I ran the example and it works great. Just a few more things.

ebidel · 2017-03-27T15:58:21Z

docs/recipes/gulp/gulpfile.js

+};
+
+const launchChrome = function() {
+  launcher = new (ChromeLauncher.ChromeLauncher || ChromeLauncher)();


is there a reason to do this? If you const ChromeLauncher = require('lighthouse/lighthouse-cli/chrome-launcher'). ChromeLauncher;, then you can just use directly.

I dunno, it was just dumb copy/paste from https://github.com/paulirish/pwmetrics/blob/master/lib/index.js#L122
maybe @paulirish knows more...

ebidel · 2017-03-27T16:04:28Z

docs/recipes/gulp/gulpfile.js

+  return connect.server({
+    root: '../public',
+    livereload: true,
+    PORT


port: PORT

ebidel · 2017-03-27T16:06:37Z

docs/recipes/gulp/gulpfile.js

+const runLighthouse = function() {
+  const url = `http://localhost:${PORT}/index.html`;
+  const ligthouseOptions = {}; // available options - https://github.com/GoogleChrome/lighthouse/#cli-options
+  lighthouse(url, ligthouseOptions, perfConfig)


return lighthouse(url, ligthouseOptions, perfConfig)

ebidel · 2017-03-27T16:10:49Z

docs/recipes/gulp/gulpfile.js

+};
+
+gulp.task('lighthouse', function() {
+  Promise.resolve(launchChrome()).then(_ => {


No need for Promise.resolve(). launchChrome() returns a promise.

launchChrome().then(_ => { connectServer(); return runLighthouse(); });

ebidel · 2017-03-27T16:19:59Z

docs/recipes/package.json

@@ -0,0 +1,12 @@
+{


I still prefer if we put everything in docs/recipes/gulp/ so this example is standalone. When we have more recipes in the future, it'll be less confusing on what npm dependencies, scripts, etc. are required for each.

Agree 👍 Just misunderstood your point

ebidel · 2017-03-27T16:20:49Z

docs/recipes/package.json

+  "devDependencies": {
+    "gulp": "^3.9.1",
+    "gulp-connect": "^5.0.0",
+    "lighthouse": "^1.6.3"


maybe less restrictive on the LH version. ^1.x might be ok, so users get the latest lighthouse when they try this demo.

ebidel · 2017-03-27T16:24:33Z

package.json

@@ -46,6 +47,7 @@
    "google-closure-compiler": "^20161201.0.0",
    "gulp": "^3.9.1",
    "gulp-concat": "^2.6.1",
+    "gulp-connect": "^5.0.0",


ebidel · 2017-03-27T16:28:32Z

package.json

    "install-cli": "cd ./lighthouse-cli && npm install",
    "install-extension": "cd ./lighthouse-extension && npm install",
    "install-viewer": "cd ./lighthouse-viewer && npm install",
+    "install-recipes": "cd ./docs/recipes && npm install",


IMO, we don't need to necessarily make recipes runnable from the main package. I see them as more as documentation examples that users can copy/paste or run themselves. Installing these extras will also slow down our good buddy Travis.

ebidel · 2017-03-27T16:30:49Z

docs/recipes/gulp/gulpfile.js

+ * Handle ok result
+ * @param {Object} results - Lighthouse results
+ */
+const handleOk = function() {


function(results) {

ebidel · 2017-03-27T16:36:45Z

docs/recipes/gulp/gulpfile.js

+ */
+const handleOk = function() {
+  disconnectServer();
+  // TODO: use lighthouse results for checking your performance expectations


Add a console.log(results); so users see something when they run the example.

brendankenny

nice! excited to get a set of recipes together

brendankenny · 2017-03-27T21:27:50Z

docs/recipes/gulp/gulpfile.js

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


need a license header

brendankenny · 2017-03-27T21:49:21Z

docs/recipes/gulp/gulpfile.js

+/**
+ * Connect to server
+ */
+const connectServer = function() {


nit: maybe startServer and stopServer instead of connectServer and disconnectServer?

brendankenny · 2017-03-27T22:24:51Z

docs/recipes/gulp/gulpfile.js

+  const lighthouseOptions = {}; // available options - https://github.com/GoogleChrome/lighthouse/#cli-options
+  return lighthouse(url, lighthouseOptions, perfConfig)
+    .then(handleOk)
+    .catch(handleError);


also a nit: should .then(handleOk).catch(handleError); be moved into the gulp.task('lighthouse') block? So it would be

gulp.task('lighthouse', function() { launchChrome().then(_ => { connectServer(); return runLighthouse() .then(handleOk); }).catch(handleError); });

or something? Seems to make more sense there than in runLighthouse

brendankenny · 2017-03-27T22:25:43Z

docs/recipes/gulp/public/index.html

@@ -0,0 +1,17 @@
+<!DOCTYPE html>


need license. Can copy the html one from e.g. https://github.com/GoogleChrome/lighthouse/blob/e822cbea89634449d66d50243fba2adce094f1d0/lighthouse-cli/test/fixtures/dobetterweb/dbw_tester.html

ebidel · 2017-03-29T04:32:41Z

docs/recipes/gulp/gulpfile.js

@@ -1,5 +1,21 @@
 'use strict';

+/**
+ * Copyright 2016 Google Inc. All rights reserved.


Sorry, 2017 :)

no bother :)

ebidel · 2017-03-29T04:33:24Z

docs/recipes/gulp/public/index.html

@@ -1,4 +1,19 @@
 <!DOCTYPE html>
+<!--
+ * Copyright 2016 Google Inc. All rights reserved.


brendankenny

LGTM2. Thanks!

If you have (or have ideas for) more helpful recipes, please keep them coming :)

denar90 · 2017-03-30T21:26:32Z

@ebidel @brendankenny thanks for grounded review and merging stuff.

If you have (or have ideas for) more helpful recipes, please keep them coming :)

Definitely 👍

add gulp recipe

38ac3c4

ebidel suggested changes Mar 25, 2017

View reviewed changes

denar90 added 2 commits March 27, 2017 14:03

improve gulp recipe

fcceed8

add pkg in recipes add install recipes script

fix eslint, add jsdoc

f78caeb

ebidel suggested changes Mar 27, 2017

View reviewed changes

cleanup

f03c4bb

brendankenny requested changes Mar 27, 2017

View reviewed changes

add license, nits

b4f8ef7

ebidel suggested changes Mar 29, 2017

View reviewed changes

copyrights year

4ede40f

ebidel approved these changes Mar 29, 2017

View reviewed changes

ebidel assigned brendankenny Mar 29, 2017

brendankenny approved these changes Mar 30, 2017

View reviewed changes

brendankenny merged commit 6898d09 into GoogleChrome:master Mar 30, 2017

denar90 deleted the recipes branch March 30, 2017 21:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add gulp recipe (wip) #1886

add gulp recipe (wip) #1886

denar90 commented Mar 21, 2017

denar90 commented Mar 22, 2017

addyosmani commented Mar 24, 2017

denar90 commented Mar 24, 2017

ebidel Mar 25, 2017

denar90 Mar 25, 2017

ebidel Mar 26, 2017

ebidel Mar 25, 2017

ebidel Mar 25, 2017

ebidel Mar 25, 2017

ebidel Mar 25, 2017

ebidel left a comment

ebidel Mar 27, 2017

denar90 Mar 27, 2017

ebidel Mar 27, 2017

ebidel Mar 27, 2017

ebidel Mar 27, 2017

ebidel Mar 27, 2017

denar90 Mar 27, 2017

ebidel Mar 27, 2017

ebidel Mar 27, 2017

ebidel Mar 27, 2017

ebidel Mar 27, 2017

ebidel Mar 27, 2017

brendankenny left a comment

brendankenny Mar 27, 2017

brendankenny Mar 27, 2017

brendankenny Mar 27, 2017

brendankenny Mar 27, 2017

ebidel Mar 29, 2017

denar90 Mar 29, 2017

ebidel Mar 29, 2017

brendankenny left a comment

denar90 commented Mar 30, 2017

add gulp recipe (wip) #1886

add gulp recipe (wip) #1886

Conversation

denar90 commented Mar 21, 2017

denar90 commented Mar 22, 2017

addyosmani commented Mar 24, 2017

denar90 commented Mar 24, 2017

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

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

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

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

brendankenny 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny left a comment

Choose a reason for hiding this comment

denar90 commented Mar 30, 2017