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: improve install progress and native unzip #18

Merged
merged 5 commits into from
Oct 9, 2023
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 71 additions & 2 deletions deno.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

106 changes: 80 additions & 26 deletions src/cache.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,11 @@
import { ensureDirSync } from "https://deno.land/[email protected]/fs/ensure_dir.ts";
import { resolve } from "https://deno.land/[email protected]/path/mod.ts";
import { ensureDir } from "https://deno.land/[email protected]/fs/ensure_dir.ts";
import { dirname } from "https://deno.land/[email protected]/path/dirname.ts";
import { join } from "https://deno.land/[email protected]/path/join.ts";
import { ZipReader } from "https://deno.land/x/[email protected]/index.js";
import ProgressBar from "https://deno.land/x/[email protected]/mod.ts";
Comment on lines +6 to +7
Copy link
Owner

Choose a reason for hiding this comment

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

Let me review these dependencies. I generally try to avoid unnecessary deps if possible.

Copy link
Owner

Choose a reason for hiding this comment

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

Alright both of these modules look fine.

import { exists } from "https://deno.land/[email protected]/fs/exists.ts";

export const SUPPORTED_VERSIONS = {
chrome: "118.0.5943.0",
Expand Down Expand Up @@ -41,28 +47,55 @@ function getCache(): Record<string, string> {
}
}

/**
* Clean cache
*/
export async function cleanCache() {
if (await exists(BASE_PATH)) {
await Deno.remove(BASE_PATH, { recursive: true });
}
}
lowlighter marked this conversation as resolved.
Show resolved Hide resolved

async function isQuietInstall() {
const { state } = await Deno.permissions.query({
name: "env",
variable: "ASTRAL_QUIET_INSTALL",
});
if (state === "granted") {
return `${Deno.env.get("ASTRAL_QUIET_INSTALL")}` === "1";
lowlighter marked this conversation as resolved.
Show resolved Hide resolved
}
}

async function decompressArchive(source: string, destination: string) {
const unzipCommand = new Deno.Command(
Deno.build.os === "windows" ? "PowerShell" : "unzip",
{
args: Deno.build.os === "windows"
? [
"Expand-Archive",
"-Path",
`"${source}"`,
"-DestinationPath",
`"${destination}"`,
"-Force",
]
: [
"-o",
source,
"-d",
destination,
],
},
);
await unzipCommand.output();
const archive = await Deno.open(source);
const zip = new ZipReader(archive);
const entries = await zip.getEntries();
const bar = new ProgressBar({
title: `Inflating ${destination}`,
total: entries.length,
clear: true,
display: ":title :bar :percent",
});
let progress = 0;
for (const entry of entries) {
if ((!entry.directory) && (entry.getData)) {
const path = join(destination, entry.filename);
await ensureDir(dirname(path));
const file = await Deno.open(path, {
create: true,
truncate: true,
write: true,
mode: 0o755,
});
await entry.getData(file, { checkSignature: true, useWebWorkers: false });
}
progress++;
bar.render(progress);
}
await zip.close();
if (!await isQuietInstall()) {
console.log(`Browser saved to ${destination}`);
}
}

/**
Expand Down Expand Up @@ -98,17 +131,38 @@ export async function getBinary(
);
})[0];

console.log(
"Downloading browser... (this may take a while depending on your internet connection)",
);
const req = await fetch(download.url);
if (!req.body) {
throw new Error(
"Download failed, please check your internet connection and try again",
);
}
await Deno.writeFile(resolve(BASE_PATH, `raw_${VERSION}.zip`), req.body);
console.log(`Download complete (${browser} version ${VERSION})`);
const reader = req.body.getReader();
const archive = await Deno.open(resolve(BASE_PATH, `raw_${VERSION}.zip`), {
write: true,
truncate: true,
create: true,
});
const bar = new ProgressBar({
title: `Downloading ${browser} ${VERSION}`,
total: Number(req.headers.get("Content-Length") ?? 0),
clear: true,
display: ":title :bar :percent",
});
let downloaded = 0;
do {
const { done, value } = await reader.read();
if (done) {
break;
}
await Deno.write(archive.rid, value);
downloaded += value.length;
bar.render(downloaded);
} while (true);
Deno.close(archive.rid);
if (!await isQuietInstall()) {
console.log(`Download complete (${browser} version ${VERSION})`);
}
Copy link
Owner

Choose a reason for hiding this comment

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

Is the performance of this about the same as writeFile? I'm a little concerned that this might be too slow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Normally it should be around the same yes (deno use something similar internally when it cannot use the op_fs directly: https://github.com/denoland/deno/blob/29026fac21d85a530d87ca3e94ae0d547557fa23/ext/fs/30_fs.js#L779-L804)
Since it's based on the ReadableStream and WritableStream, there shouldn't be too much as the operation is similar to a pipeTo(), it's just that it peak at the chunk length while the stream is read but since it doesn't copy data into intermediate buffer I don't think it impact performances that much

But maybe for the quiet install we could optimize and fallback on the previous way of writing?

Copy link
Owner

Choose a reason for hiding this comment

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

Sure. I think quiet install going back to the other path makes sense to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

I made a small benchmark just to check if it was correct, seems that there isn't a big difference.
I feel like it's worth it to have the progress so you know the programm isn't stuck, but lmk if you'd prefer this to not be the default behaviour

Copy link
Owner

Choose a reason for hiding this comment

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

I think this is valuable. Thanks for the work. Let me review dependencies and I think this PR looks good. I'll give it one more lookover.

await decompressArchive(
resolve(BASE_PATH, `raw_${VERSION}.zip`),
resolve(BASE_PATH, VERSION),
Expand Down
30 changes: 30 additions & 0 deletions tests/_get_binary_test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
import { assertMatch } from "https://deno.land/[email protected]/assert/assert_match.ts";
import { cleanCache, getBinary, launch } from "../mod.ts";
import { assert } from "https://deno.land/[email protected]/assert/assert.ts";

Deno.test("Test download", async () => {
// Download browser
await cleanCache();
const path = await getBinary("chrome");

// Ensure browser is executable
// Note: it seems that on Windows the --version flag does not exists and spawn a
// browser instance instead. The next test ensure that everything is working
// properly anyways
if (Deno.build.os !== "windows") {
const command = new Deno.Command(path, {
args: [
"--version",
],
});
const { success, stdout } = await command.output();
assert(success);
assertMatch(new TextDecoder().decode(stdout), /Google Chrome/i);
}

// Ensure browser is capable of loading pages
const browser = await launch();
const page = await browser.newPage("http://example.com");
await page.waitForSelector("h1");
await browser.close();
});
lowlighter marked this conversation as resolved.
Show resolved Hide resolved
Loading