Add chrome hostname to CriConnection & eslint fix #2728

FrederickGeek8 · 2017-07-24T00:19:26Z

This issue fulfills the request made in issue #2727. This change should allow for Chrome to be located on a hostname other than localhost, as outlined in the issue. It may be noted that I added the hostname parameter to be the second parameter in the constructor so that it does not break any previous implementations of CriConnection.

Additionally, I made a small change that allowed for the yarn tests to be passed. No functionality was changed in this (the second) commit.

Decided to make hostname second parameter to avoid breaking existing implementations

wardpeet · 2017-07-24T15:06:06Z

TBH I rather have the eslint fix out of this PR but not my call to make.

FrederickGeek8 · 2017-07-24T15:08:05Z

Tests weren't compiling without it so I saw it as a necessary fix. I can attempt to remove it if requested however...

wardpeet · 2017-07-24T15:16:55Z

lighthouse-core/gather/connections/cri.js

 const CONNECT_TIMEOUT = 10000;
 const DEFAULT_PORT = 9222;

 class CriConnection extends Connection {
  /**
   * @param {number=} port Optional port number. Defaults to 9222;
+   * @param {string} hostname Optional hostname. Defaults to localhost.


@param {string} should be @param {string=} as we use closure compiler for our minification.

ref: https://github.com/google/closure-compiler/wiki/Annotating-JavaScript-for-the-Closure-Compiler#optional-parameter-in-a-param-annotation

wardpeet · 2017-07-24T15:24:22Z

@FrederickGeek8 nonetheless thanks for the PR! 🎉

Lets wait for @paulirish @patrickhulce @brendankenny to pick this review up

FrederickGeek8 · 2017-07-24T15:28:05Z

Sorry about that, I thought I put that in. Should be fixed now.

patrickhulce · 2017-07-24T20:34:32Z

Awesome thanks so much for jumping on this @FrederickGeek8!

What do you think about plumbing it through to https://github.com/GoogleChrome/lighthouse/blob/master/lighthouse-core/index.js#L38 (read from flags.hostname) as well so the programmatic use case will be covered?

I think…

FrederickGeek8 · 2017-07-24T20:54:31Z

Like so? Also, do you think that anything should be added to documentation?

patrickhulce · 2017-07-24T21:06:42Z

perfect👌 and great question!

Adding this to the CLI will give us the CLI documentation for free (which you can then use to update the copy of the output found in the README).

We don't really have comprehensive documentation of the programmatic flags yet, but you could add a nod to setting flags.host over in the first example in using programmatically docs for now.

FrederickGeek8 · 2017-07-24T22:18:05Z

I added some documentation to the mentioned locations. I also fixed the PR conflict.

paulirish · 2017-07-31T20:21:38Z

Presumably this does indeed work? I had thought the remote-debugging-port was only listening for localhost connections. Was there another CLI flag you needed to pass to relax that, or am i wrong?

patrickhulce · 2017-07-31T20:27:21Z

Presumably this does indeed work? I had thought the remote-debugging-port was only listening for localhost connections. Was there another CLI flag you needed to pass to relax that, or am i wrong?

Maybe there's a special way docker works its magic for networking containers together? I seem to remember it having something unique going on.

brendankenny · 2017-07-31T21:11:55Z

Sounds plausible. @FrederickGeek8 or @salvocanna can you comment to clarify your use case/workflow?

salvocanna · 2017-08-01T08:38:06Z

@paulirish indeed, I'm using yukinying/chrome-headless-browser-docker and this line forces the bind to 0.0.0.0 so allowing external connections.

I'm using one docker container with the full application in node (lighthouse included) and another container to host the actual browser. Seems to work fine updating the hostname.

brendankenny · 2017-08-01T21:48:58Z

docs/readme.md

@@ -13,6 +13,7 @@ const chromeLauncher = require('lighthouse/chrome-launcher');

 function launchChromeAndRunLighthouse(url, flags = {}, config = null) {
  return chromeLauncher.launch().then(chrome => {
+    flags.host = 'localhost';


I think we should actually remove this, sorry :) We don't document any of the other possible flags here, and we want to keep this example as simple as possible for copy/pasting to get started.

We should start a separate section to document the flags (or at least point to the CLI flags as the same as what this accepts), but that's a change for a different PR.

brendankenny · 2017-08-01T21:49:56Z

lighthouse-cli/cli-flags.ts

-      logLevel: string
+  port: number, host: string, chromeFlags: string, output: any, outputPath: string,
+      interactive: boolean, saveArtifacts: boolean, saveAssets: boolean, view: boolean,
+      maxWaitForLoad: number, logLevel: string


the diff would be a lot clearer (just adding host: string) if this had just been appended on the end here, but no biggie :)

brendankenny · 2017-08-01T21:51:41Z

lighthouse-viewer/app/src/github-api.js

@@ -116,7 +116,9 @@ class GithubApi {
            const filename = Object.keys(json.files)
                .find(filename => filename.endsWith(GithubApi.LH_JSON_EXT));
            if (!filename) {
-              throw new Error(`Failed to find a Lighthouse report (*${GithubApi.LH_JSON_EXT}) in gist ${id}`);
+              throw new Error(


drop this change from the PR?

This was to fix a conflict that was introduced from a different PR somewhere along the road.

brendankenny · 2017-08-01T21:55:52Z

lighthouse-core/gather/connections/cri.js

   * @constructor
   */
-  constructor(port) {
+  constructor(port, hostname) {


I think this is an older file, so it doesn't use them yet, but do you want to switch these two to use default parameters instead of ||?

Slight difference in port since it won't turn 0 into 9222, but we don't support port 0 at this stage anyways (trying to find a debugger on a random port makes no sense)

brendankenny · 2017-08-01T21:58:08Z

lighthouse-cli/cli-flags.ts

@@ -77,6 +77,7 @@ export function getFlags(manualArgv?: string) {
            CHROME_PATH: Explicit path of intended Chrome binary. If set must point to an executable of a build of Chromium version 54.0 or later. By default, any detected Chrome Canary or Chrome (stable) will be launched.
            `,
        'perf': 'Use a performance-test-only configuration',
+        'host': 'The host to use for the debugging protocol.',


should copy this to https://github.com/GoogleChrome/lighthouse/blob/master/readme.md#cli-options, too (like #2757 just did for CHROME_PATH)

Also remove comments from docs/readme.md, add comment to readme.md, and make cli-flags.ts change more digestible.

brendankenny · 2017-08-03T22:26:45Z

lighthouse-core/gather/connections/cri.js

   * @constructor
   */
-  constructor(port) {
+  constructor(port = DEFAULT_PORT, hostname = DEFAULT_HOSTNAME) {


these still need to be saved on this.port and this.hostname (cause of the test failures)

FrederickGeek8 · 2017-08-11T04:48:28Z

@brendankenny Just a heads up, I made the requested changes.

brendankenny

sorry for being slow; thanks for the ping. LGTM!

googlebot · 2017-08-14T21:16:21Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

paulirish · 2017-08-14T21:20:18Z

I renamed host to hostname everywhere, but this is now landed.

FrederickGeek8 added 2 commits July 23, 2017 20:09

Fulfills GoogleChrome#2727

cfd8e3f

Decided to make hostname second parameter to avoid breaking existing implementations

Add fix to pass tests

eace435

FrederickGeek8 mentioned this pull request Jul 24, 2017

Fix eslint issue max chars in github-api.js #2730

Merged

FrederickGeek8 changed the title ~~Add chrome hostname to CriConnection~~ Add chrome hostname to CriConnection & eslint fix Jul 24, 2017

wardpeet reviewed Jul 24, 2017

View reviewed changes

Fix annotation

3db0771

Add programmatic use case

f224ab3

I think…

FrederickGeek8 added 2 commits July 24, 2017 17:52

Add documentation

d056543

Fix PR conflicts

508b671

salvocanna mentioned this pull request Jul 29, 2017

Add hostname as parameter for GoogleChrome/lighthouse#2727 #2793

Closed

brendankenny requested changes Aug 1, 2017

View reviewed changes

Add ES6 default parameters

e87d50f

Also remove comments from docs/readme.md, add comment to readme.md, and make cli-flags.ts change more digestible.

paulirish assigned brendankenny Aug 2, 2017

brendankenny reviewed Aug 3, 2017

View reviewed changes

Fix tests

284d5a7

brendankenny approved these changes Aug 14, 2017

View reviewed changes

rename host to hostname for spec correctness

df95aaa

paulirish merged commit 0455283 into GoogleChrome:master Aug 14, 2017

tkindy mentioned this pull request Feb 25, 2021

CLI tries to launch a Chrome instance even when given a remote hostname #12137

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add chrome hostname to CriConnection & eslint fix #2728

Add chrome hostname to CriConnection & eslint fix #2728

FrederickGeek8 commented Jul 24, 2017 •

edited

Loading

wardpeet commented Jul 24, 2017

FrederickGeek8 commented Jul 24, 2017 •

edited

Loading

wardpeet Jul 24, 2017

wardpeet commented Jul 24, 2017

FrederickGeek8 commented Jul 24, 2017 •

edited

Loading

patrickhulce commented Jul 24, 2017

FrederickGeek8 commented Jul 24, 2017

patrickhulce commented Jul 24, 2017

FrederickGeek8 commented Jul 24, 2017

paulirish commented Jul 31, 2017

patrickhulce commented Jul 31, 2017

brendankenny commented Jul 31, 2017

salvocanna commented Aug 1, 2017 •

edited

Loading

brendankenny Aug 1, 2017

brendankenny Aug 1, 2017

brendankenny Aug 1, 2017

FrederickGeek8 Aug 2, 2017

brendankenny Aug 1, 2017

brendankenny Aug 1, 2017

brendankenny Aug 3, 2017

FrederickGeek8 Aug 3, 2017

FrederickGeek8 commented Aug 11, 2017

brendankenny left a comment

googlebot commented Aug 14, 2017

paulirish commented Aug 14, 2017

Add chrome hostname to CriConnection & eslint fix #2728

Add chrome hostname to CriConnection & eslint fix #2728

Conversation

FrederickGeek8 commented Jul 24, 2017 • edited Loading

wardpeet commented Jul 24, 2017

FrederickGeek8 commented Jul 24, 2017 • edited Loading

Choose a reason for hiding this comment

wardpeet commented Jul 24, 2017

FrederickGeek8 commented Jul 24, 2017 • edited Loading

patrickhulce commented Jul 24, 2017

FrederickGeek8 commented Jul 24, 2017

patrickhulce commented Jul 24, 2017

FrederickGeek8 commented Jul 24, 2017

paulirish commented Jul 31, 2017

patrickhulce commented Jul 31, 2017

brendankenny commented Jul 31, 2017

salvocanna commented Aug 1, 2017 • edited Loading

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

FrederickGeek8 commented Aug 11, 2017

brendankenny left a comment

Choose a reason for hiding this comment

googlebot commented Aug 14, 2017

paulirish commented Aug 14, 2017

FrederickGeek8 commented Jul 24, 2017 •

edited

Loading

FrederickGeek8 commented Jul 24, 2017 •

edited

Loading

FrederickGeek8 commented Jul 24, 2017 •

edited

Loading

salvocanna commented Aug 1, 2017 •

edited

Loading