feat: Introduce BrowserFetcher class #1983
Conversation
This patch introduces `Downloader` class that manages downloaded versions of products. This patch: - shapes Downloader API to be minimal yet usable for our needs. This includes removing such methods as `Downloader.supportedPlatforms` and `Downloader.defaultRevision`. - makes most of the fs-related methods in Downloader async. The only exception is the `Downloader.revisionInfo`: it has stay sync due to the `pptr.executablePath()` method being sync. - updates `install.js` and `utils/check_availability.js` to use new API. Fixes puppeteer#1748.
lib/Downloader.js
Outdated
| class Downloader { | ||
| /** | ||
| * @return {!Array<string>} | ||
| * @param {!Object=} options |
There was a problem hiding this comment.
can we jsdoc these options? It's a bit hard to read otherwise.
docs/api.md
Outdated
| The method initiates a HEAD request to check if revision is available. | ||
|
|
||
| #### downloader.downloadRevision(revision[, progressCallback]) | ||
| - `revision` <[string]> a revision to check availability. |
There was a problem hiding this comment.
a revision to download.
docs/api.md
Outdated
|
|
||
| Downloader is created per platform and operates revisions: strings representing 6-digit numbers, e.g. `"533271"`. Revisions could be checked with [omahaproxy.appspot.com](http://omahaproxy.appspot.com/). | ||
|
|
||
| > **NOTE** Downloader class is not designed to work concurrently with other instances of Downloader class. |
There was a problem hiding this comment.
This makes me want to have it as a namespace with configurable options.
There was a problem hiding this comment.
This constrained can be removed with a lock file if we need to. I'd keep it as implementation detail without hardcoding into the API: it shouldn't yield much confusion.
There was a problem hiding this comment.
The Downloader class ... of the Downloader class
docs/api.md
Outdated
| - returns: <[Promise]> Resolves when the revision is downloaded | ||
|
|
||
| #### downloader.revisionInfo(revision) | ||
| - `revision` <[string]> a revision to get info. |
| else if (platform === 'linux') | ||
| this._platform = 'linux'; | ||
| else if (platform === 'win32') | ||
| this._platform = os.arch() === 'x64' ? 'win64' : 'win32'; |
There was a problem hiding this comment.
console.assert(this._platform, 'Unsupported platform: ' + os.platform());
docs/api.md
Outdated
| - `folderPath` <[string]> path to the extracted revision folder | ||
| - `executablePath` <[string]> path to the revision executable | ||
| - `url` <[string]> URL this revision can be downloaded from | ||
| - `downloaded` <[boolean]> weather the revision is downloaded or not |
docs/api.md
Outdated
|
|
||
| ### class: Downloader | ||
|
|
||
| A Downloader is manages downloaded chromium instances. |
There was a problem hiding this comment.
A Downloader can download and manage different versions of Chromium.
docs/api.md
Outdated
|
|
||
| A Downloader is manages downloaded chromium instances. | ||
|
|
||
| Downloader is created per platform and operates revisions: strings representing 6-digit numbers, e.g. `"533271"`. Revisions could be checked with [omahaproxy.appspot.com](http://omahaproxy.appspot.com/). |
There was a problem hiding this comment.
The Downloader operates on revision strings that specify a precise version of Chromium, e.g. "533271". Revision strings can be obtained from omahaproxy.appspot.com.
docs/api.md
Outdated
| The method initiates a GET request to download revision from the host. | ||
|
|
||
| #### downloader.downloadedRevisions() | ||
| - returns: <[Promise]<[Array]<[string]>>> a list of all revisions downloaded for the given platform |
There was a problem hiding this comment.
storedRevisions
A list of all revisions previously downloaded for the current platform.
docs/api.md
Outdated
| #### downloader.downloadRevision(revision[, progressCallback]) | ||
| - `revision` <[string]> a revision to download. | ||
| - `progressCallback` <[function]> A function that will be called with two arguments: | ||
| - `downloadedBytes` <[number]> how many bytes are downloaded |
docs/api.md
Outdated
| - `revision` <[string]> a revision to download. | ||
| - `progressCallback` <[function]> A function that will be called with two arguments: | ||
| - `downloadedBytes` <[number]> how many bytes are downloaded | ||
| - `totalBytes` <[number]> how many bytes is the total download |
There was a problem hiding this comment.
How large is the total download.
docs/api.md
Outdated
| - `options` <[Object]> | ||
| - `host` <[string]> A download host to be used. Defaults to `https://storage.googleapis.com`. | ||
| - `path` <[string]> A path for the downloads folder. Defaults `<root>/.local-chromium`, where `<root>` is puppeteer's package root. | ||
| - `platform` <[string]> Specify binary type. Possible values are: `mac`, `win32`, `win64`, `linux`. Defaults to the current platform. |
There was a problem hiding this comment.
Drop: Specify binary type.
docs/api.md
Outdated
| #### puppeteer.createDownloader([options]) | ||
| - `options` <[Object]> | ||
| - `host` <[string]> A download host to be used. Defaults to `https://storage.googleapis.com`. | ||
| - `path` <[string]> A path for the downloads folder. Defaults `<root>/.local-chromium`, where `<root>` is puppeteer's package root. |
docs/api.md
Outdated
| - `revision` <[string]> a revision to check availability. | ||
| - returns: <[Promise]<[boolean]>> returns `true` if the revision could be downloaded from the host. | ||
|
|
||
| The method initiates a HEAD request to check if revision is available. |
docs/api.md
Outdated
|
|
||
| #### downloader.downloadRevision(revision[, progressCallback]) | ||
| - `revision` <[string]> a revision to download. | ||
| - `progressCallback` <[function]> A function that will be called with two arguments: |
There was a problem hiding this comment.
<[function]([number], [number])>
docs/api.md
Outdated
| - `totalBytes` <[number]> how many bytes is the total download | ||
| - returns: <[Promise]> Resolves when the revision is downloaded and extracted | ||
|
|
||
| The method initiates a GET request to download revision from the host. |
There was a problem hiding this comment.
This method ... the revision
docs/api.md
Outdated
|
|
||
| #### downloader.removeRevision(revision) | ||
| - `revision` <[string]> a revision to remove. The method will throw if the revision is not downloaded. | ||
| - returns: <[Promise]> Resolves when the revision is downloaded |
docs/api.md
Outdated
| - returns: <[string]> Downloader target platform. Returns one of the `mac`, `linux`, `win32` or `win64`. | ||
|
|
||
| #### downloader.removeRevision(revision) | ||
| - `revision` <[string]> a revision to remove. The method will throw if the revision is not downloaded. |
There was a problem hiding this comment.
has not been downloaded.
docs/api.md
Outdated
| - returns: <[Promise]<[Array]<[string]>>> a list of all revisions downloaded for the given platform | ||
|
|
||
| #### downloader.platform() | ||
| - returns: <[string]> Downloader target platform. Returns one of the `mac`, `linux`, `win32` or `win64`. |
There was a problem hiding this comment.
Returns one of the ...
docs/api.md
Outdated
| - `folderPath` <[string]> path to the extracted revision folder | ||
| - `executablePath` <[string]> path to the revision executable | ||
| - `url` <[string]> URL this revision can be downloaded from | ||
| - `downloaded` <[boolean]> wether the revision is downloaded or not |
There was a problem hiding this comment.
has been downloaded or not
| const downloader = Downloader.createDefault(); | ||
| const downloadHost = process.env.PUPPETEER_DOWNLOAD_HOST || process.env.npm_config_puppeteer_download_host; | ||
|
|
||
| const puppeteer = require('./index'); |
There was a problem hiding this comment.
Does this work on node 6???
There was a problem hiding this comment.
Of course! the index.js is the one that checks for async/await support and picks the right puppeteer.
There was a problem hiding this comment.
What happens if you checkout from the GitHub and run npm install? Do we built before downloading somewhere?
There was a problem hiding this comment.
@JoelEinbinder yes, we build in the very first line of the script
docs/api.md
Outdated
| - `host` <[string]> A download host to be used. Defaults to `https://storage.googleapis.com`. | ||
| - `path` <[string]> A path for the downloads folder. Defaults to `<root>/.local-chromium`, where `<root>` is puppeteer's package root. | ||
| - `platform` <[string]> Possible values are: `mac`, `win32`, `win64`, `linux`. Defaults to the current platform. | ||
| - returns: <[Downloader]> |
There was a problem hiding this comment.
Missing bottom reference?
| expect(await downloader.canDownload('123456')).toBe(true); | ||
|
|
||
| await downloader.download('123456'); | ||
| expect(await readFileAsync(revisionInfo.executablePath, 'utf8')).toBe('LINUX BINARY\n'); |
There was a problem hiding this comment.
the executablePath is defined before the binary is downloaded?
There was a problem hiding this comment.
yes, the revisionInfo could be called for both downloaded and non-downloaded revisions.
There was a problem hiding this comment.
This is the expected use, right?
// I want to use a different revision than the one in package.json...
const downloader = puppeteer.createDownloader();
await downloader.download('621459');
const revisionInfo = downloader.revisionInfo('621459');
await puppeteer.launch({executablePath: revisionInfo.executablePath})
docs/api.md
Outdated
| - `folderPath` <[string]> path to the extracted revision folder | ||
| - `executablePath` <[string]> path to the revision executable | ||
| - `url` <[string]> URL this revision can be downloaded from | ||
| - `downloaded` <[boolean]> wether the revision has been downloaded or not |
docs/api.md
Outdated
| - `progressCallback` <[function]([number], [number])> A function that will be called with two arguments: | ||
| - `downloadedBytes` <[number]> how many bytes have been downloaded | ||
| - `totalBytes` <[number]> how large is the total download. | ||
| - returns: <[Promise]> Resolves when the revision is downloaded and extracted |
There was a problem hiding this comment.
should it return revisionInfo?
seems optional but kinda nice.
There was a problem hiding this comment.
I like returning revisionInfo, done.
docs/api.md
Outdated
| - `folderPath` <[string]> path to the extracted revision folder | ||
| - `executablePath` <[string]> path to the revision executable | ||
| - `url` <[string]> URL this revision can be downloaded from | ||
| - `downloaded` <[boolean]> wether the revision has been downloaded or not |
There was a problem hiding this comment.
if i download and then remove a rev, what will revInfo.downloaded tell me? does this property really mean "currently on disk" ? maybe available?
also if i do any of these actions in separate downloader instances (assuming same host/path/platform), does it matter?
There was a problem hiding this comment.
if i download and then remove a rev, what will revInfo.downloaded tell me? does this property really mean "currently on disk" ?
Yes, that's 'currently on disk'.
also if i do any of these actions in separate downloader instances (assuming same host/path/platform), does it matter?
There are no guarantees in this case. If two downloaders try to download the same revision simultaneously, they will fight with each other and fail in some weird way.
There's a note about this in the description of downloader class.
@paulirish yeah. Also I'd expect different scripts for downloading and running, like we have Also, with the |
docs/api.md
Outdated
|
|
||
| ### class: Downloader | ||
|
|
||
| A Downloader can download and manage different versions of Chromium. |
There was a problem hiding this comment.
ChromiumDownloader? At first I thought this PR was for handing file downloads, but I see that's it for downloading Chromium. I wonder if the name should be more explicit?
createDownloader -> createChromiumDownloader too. Thoughts?
There was a problem hiding this comment.
@ebidel in a follow-up, I'll teach it to download content_shell builds, so it's not solely about chromium.
This patch introduces
BrowserFetcherclass that managesdownloaded versions of products.
This patch:
includes removing such methods as
Downloader.supportedPlatformsandDownloader.defaultRevision.exception is the
Downloader.revisionInfo: it has stay sync due to thepptr.executablePath()method being sync.install.jsandutils/check_availability.jsto use new APIDownloaderintoBrowserFetcherFixes #1748.