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

WIP: use chrome-launcher for handling the browser. #23

Closed
wants to merge 1 commit into from

Conversation

paulirish
Copy link
Contributor

@paulirish paulirish commented Jun 19, 2017

this is the launcher module from lighthouse that we maintain.
https://www.npmjs.com/package/chrome-launcher

it defaults to an open port

addresses #21

Copy link
Contributor

@aslushnikov aslushnikov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's exciting!

lib/Browser.js Outdated

await waitForChromeResponsive(this._remoteDebuggingPort);
// this._chromeProcess.on('exit', () => {
// this._terminated = true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we have this back? I think it's still valuable

@@ -16,6 +16,7 @@
"author": "The Chromium Authors",
"license": "SEE LICENSE IN LICENSE",
"dependencies": {
"chrome-launcher": "^0.1.3",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does chrome-launcher depend on lighthouse? (i see it here) This seems to be a pretty heavy dependency to pull in, is it really needed?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR landing very soon to fix that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

chrome-launcher@0.2.0 shipped without LH as a dep.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed.

lib/Browser.js Outdated
@@ -36,15 +38,9 @@ class Browser {
*/
constructor(options) {
options = options || {};
++browserId;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's also kill it from L26

@paulirish
Copy link
Contributor Author

Updated and rebased.

However we still need launchedChrome.on('exit', fn) support from chrome-launcher to complete this.

@paulirish
Copy link
Contributor Author

Closing until this is ready.

@paulirish paulirish closed this Jul 11, 2017
@aslushnikov aslushnikov deleted the chromelauncher branch August 1, 2017 19:17
@0xtoorich
Copy link

Hi, can we use chrome-launcher with puppeteer now?

@0xtoorich
Copy link

I copy some code from chrome-launcher to make it work, but it looks so dirty. Is there a better way to archive that? Thanks. 😉

const { getPlatform } = require('chrome-launcher/utils')
const chromeFinder = require('chrome-launcher/chrome-finder')
const Puppeteer = require('puppeteer')

async function setOptionsDefaults(options)
{
  if (options.executablePath === undefined) {
    const installations = await chromeFinder[getPlatform()]()
    if (installations.length === 0) {
      throw new Error('No Chrome Installations Found')
    }
    options.executablePath = installations[0]
  }

  return options
}

class Puppeteer2 extends Puppeteer {
  static async launch(options) {
    options = await setOptionsDefaults(options)
    return super.launch(options)
  }
}

module.exports = Puppeteer2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants