-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
Add chrome hostname to CriConnection & eslint fix #2728
Changes from 7 commits
cfd8e3f
eace435
3db0771
f224ab3
d056543
508b671
e87d50f
284d5a7
df95aaa
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,19 +10,18 @@ const WebSocket = require('ws'); | |
const http = require('http'); | ||
const log = require('lighthouse-logger'); | ||
|
||
const hostname = 'localhost'; | ||
const DEFAULT_HOSTNAME = 'localhost'; | ||
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. | ||
* @constructor | ||
*/ | ||
constructor(port) { | ||
constructor(port = DEFAULT_PORT, hostname = DEFAULT_HOSTNAME) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. these still need to be saved on There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My bad... |
||
super(); | ||
|
||
this.port = port || DEFAULT_PORT; | ||
} | ||
|
||
/** | ||
|
@@ -77,7 +76,7 @@ class CriConnection extends Connection { | |
_runJsonCommand(command) { | ||
return new Promise((resolve, reject) => { | ||
const request = http.get({ | ||
hostname: hostname, | ||
hostname: this.hostname, | ||
port: this.port, | ||
path: '/json/' + command | ||
}, response => { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. drop this change from the PR? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was to fix a conflict that was introduced from a different PR somewhere along the road. |
||
`Failed to find a Lighthouse report (*${GithubApi.LH_JSON_EXT}) in gist ${id}` | ||
); | ||
} | ||
const f = json.files[filename]; | ||
if (f.truncated) { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should copy this to https://github.com/GoogleChrome/lighthouse/blob/master/readme.md#cli-options, too (like #2757 just did for
CHROME_PATH
)