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

(feat) Add option to fetch Firefox Nightly #5467

Merged
merged 9 commits into from Mar 10, 2020

Conversation

mjzffr
Copy link
Contributor

@mjzffr mjzffr commented Feb 28, 2020

Add Firefox support to BrowserFetcher and the install script.
By default, the latest Firefox Nightly is downloaded
directly from archive.mozilla.org (dmg, tar.bz2 and zip)

This also required changes that impact puppeteer.launch()
and puppeteer.executablePath()

Fixes #5151

Add Firefox support to BrowserFetcher and the install script.
By default, the latest Firefox Nightly is downloaded
directly from archive.mozilla.org (dmg, tar.bz2 and zip)

This also required changes that impact `puppeteer.launch()`
and `puppeteer.executablePath()`

Fixes puppeteer#5151
@@ -37,13 +37,13 @@ npm i puppeteer
# or "yarn add puppeteer"
```

Note: When you install Puppeteer, it downloads a recent version of Chromium (~170MB Mac, ~282MB Linux, ~280MB Win) that is guaranteed to work with the API. To skip the download, see [Environment variables](https://github.com/puppeteer/puppeteer/blob/v2.1.1/docs/api.md#environment-variables).
Note: When you install Puppeteer, it downloads a recent version of Chromium (~170MB Mac, ~282MB Linux, ~280MB Win) that is guaranteed to work with the API. To skip the download, or to download a different browser, see [Environment variables](https://github.com/puppeteer/puppeteer/blob/v2.1.1/docs/api.md#environment-variables).
Copy link
Member

Choose a reason for hiding this comment

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

For the record, we probably don’t want to do this right now, but in the future we could work towards npm install puppeteer fetching both Chromium and Firefox binaries by default. I like your approach of taking one step at a time 👍🏻

docs/api.md Outdated Show resolved Hide resolved
@mathiasbynens
Copy link
Member

I’ll take a closer look during the week, but just wanted to drop a note saying I’m super excited about this!

docs/api.md Outdated Show resolved Hide resolved
Copy link
Member

@mathiasbynens mathiasbynens left a comment

Choose a reason for hiding this comment

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

Generally looks good, just some questions and suggestions. @hanselfmu could you PTAL as well, especially w.r.t. dropping Node.js v8?

docs/api.md Outdated Show resolved Hide resolved
install.js Outdated Show resolved Hide resolved
}).on('error', e => reject(e));

function parseVersion() {
const regex = /firefox\-(?<version>\d\d)\..*/gm;
Copy link
Member

Choose a reason for hiding this comment

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

Loving the use of named capture groups here!

This will fail on Node.js v8.x.x so we'd want to land ##5365 first. @hanselfmu could you please take another look?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The PR to drop Node.js v8.x support is labeled as a breaking change, which means landing it will be in Puppeteer v3. I think we have two options now:

  1. land both of them in v3;
  2. temporarily use Node.js-8-compatible code for now, and when v3 lands, update the legacy code.
    @mjzffr what do you think? Do you have other ideas?

Copy link
Member

Choose a reason for hiding this comment

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

IMHO major version bumps are no big deal. We shouldn't hold back on publishing v3.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sounds good. I'll publish v3 now and include this PR.

Copy link

Choose a reason for hiding this comment

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

@hanselfmu @mathiasbynens What happened with this? This PR doesn't seem to be available in a release version yet.

install.js Outdated Show resolved Hide resolved
test/launcher.spec.js Outdated Show resolved Hide resolved
mjzffr and others added 8 commits March 2, 2020 09:59
Co-Authored-By: Mathias Bynens <mathias@qiwi.be>
Co-Authored-By: Mathias Bynens <mathias@qiwi.be>
Co-Authored-By: Mathias Bynens <mathias@qiwi.be>
@mjzffr
Copy link
Contributor Author

mjzffr commented Mar 9, 2020 via email

Copy link
Collaborator

@hanselfmu hanselfmu left a comment

Choose a reason for hiding this comment

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

LGTM

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

Successfully merging this pull request may close these issues.

(Laucher, BrowserFetcher) Download appropriate Firefox version; determine executable path
5 participants