feat: improve install progress and native unzip #18
Conversation
|
It looks like the windows test is hanging on something. Any ideas why? |
Yeah I just debugged on my laptop, for some reason |
|
That is quite strange, I've noticed a lot of inconsistencies between browsers and different operating systems. |
src/cache.ts
Outdated
| 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})`); | ||
| } |
There was a problem hiding this comment.
Is the performance of this about the same as writeFile? I'm a little concerned that this might be too slow.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Sure. I think quiet install going back to the other path makes sense to me.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
| import { ZipReader } from "https://deno.land/x/[email protected]/index.js"; | ||
| import ProgressBar from "https://deno.land/x/[email protected]/mod.ts"; |
There was a problem hiding this comment.
Let me review these dependencies. I generally try to avoid unnecessary deps if possible.
There was a problem hiding this comment.
Alright both of these modules look fine.
| const ci = await Deno.permissions.query({ | ||
| name: "env", | ||
| variable: "CI", | ||
| }); | ||
| if ((ci.state === "granted") && (`${Deno.env.get("CI") ?? ""}`.length)) { | ||
| return true; | ||
| } |
|
Thanks for your great work! |
This PR removes the need of
--allow-run=powershell(greatly reducing attack vectors for Windows) and--allow-run=unzipby using native JS instead.It also add some QOL features, such as a progress bar for both downloading/inflating browser archive, along with
ASTRAL_QUIET_INSTALLenv var that hide Astral'sconsole.log. Also addedcleanCache()function so it's possible to add browser install to the unit tests