chore: Replace extract-zip with yauzl-promise #1527
Conversation
|
|
|
extract-zip is abandonware? It looks like it was last updated 3 hours ago. |
|
Overall this makes sense though. I like any patch that reduces our dependency count. Can we easily move to yauzl directly instead of wrapping it with yauzl-promise? |
|
Oh wow, extract-zip finally updated! |
Looks like typescript gets confused because open has multiple signatures. Try this: const open = util.promisify(yauzl.open as (path: string, options: yauzl.Options, callback?: (err?: Error, zipfile?: yauzl.ZipFile) => void) => void);Otherwise I don't think node callbacks are a big deal. Can just wrap them in a quick promise yourself. Thanks! |
|
@ABuffSeagull I'm eagerly looking forward for this to land! Do you plan to continue working on this, or do you want me to take over? |
|
Don't worry, I'll have this done today. I just have more free time during weekdays than weekends for some reason. |
|
Okay, hopefully should be all good now? |
There was a problem hiding this comment.
@ABuffSeagull: please accept my apology for the slow response.
I looked into this, and it looks like we need to update node types first. The PR for this is here: #1690
Once it lands, we'll proceed with this one.
As for the code itself, looks good! I left a few minor comments in-line.
src/server/browserFetcher.ts
Outdated
| @@ -28,6 +29,8 @@ import { assert } from '../helper'; | |||
|
|
|||
| const unlinkAsync = util.promisify(fs.unlink.bind(fs)); | |||
| const chmodAsync = util.promisify(fs.chmod.bind(fs)); | |||
| const mkdirAsync = fs.promises.mkdir; | |||
There was a problem hiding this comment.
let's not use fs.promises - I think they're still experimental in node 10.
Instead, we can use good-old util.promisify:
const mkdirAsync = util.promisify(fs.mkdir.bind(fs));
|
No problem, it's all good |
|
@ABuffSeagull the #1690 has landed - can you please re-baseline atop of tip-of-tree? |
Since extract-zip is abandonware, remove it and just work with yauzl directly. Fixes microsoft#1510
|
@ABuffSeagull thank you for the followup! Unfortunately, it looks like it doesn't unzip webkit on linux properly, probably due to symlinks. Looking at the extract-zip source code, it seems to me that most of this functionality is actually required here. I'm sorry I pulled you into this - I got overly excited to drop some unnecessary complexity, but it looks like it is inevitable. At this point, I'd rather just bump Thank you for your contribution though! And again, sorry to spend your time without much success here. |
|
Ah, if only I had a Mac, I could've realized earlier. And no problem! It was pretty fun figuring this out and you were very helpful! |
Since extract-zip is abandonware, remove it and just work with
yauzl directly.
In order to make TypeScript happy with recursive
mkdir, I updated@types/nodeto v10 (since the min version for playwright is v10 anyway). However, this broke some other typings throughout the code.Fixes #1510