search

nodejs / readable-stream

Node-core streams for userland
https://nodejs.org/api/stream.html
Other
1.03k stars 227 forks source link

chore: Move playwright to optionalDependencies. #476

Closed ShogunPanda closed 2 years ago

ShogunPanda commented 2 years ago

Fixes #475 and #473.

mcollina commented 2 years ago

Can you put a timeouton the browser workflows?

ShogunPanda commented 2 years ago

@mcollina Done!

vweevers commented 2 years ago

@mcollina I don't fully understand why you don't want to install playwright. Locally you'll only need to download the playwright binaries once, after that they're cached. Is it about CI? If so, you can selectively set PLAYWRIGHT_SKIP_BROWSER_DOWNLOAD=1

ShogunPanda commented 2 years ago

@vweevers Yup, I think is to avoid to block CI in case those outages happen again.

In general, since downloading browsers might be expensive (large files on mobile networks) I think we should not do it by default.

mcollina commented 2 years ago

Maybe this is a bad idea and we can just skip this? Everything seems to be actually working fine now.

vweevers commented 2 years ago

+1 to skipping. I don't think this is worth the cost (like having to update the playwright version in a non-standard place). Can be revisited if there are similar issues in the future (but if they're incidental then PLAYWRIGHT_SKIP_BROWSER_DOWNLOAD=1 should suffice).

ShogunPanda commented 2 years ago

Sounds good to me! Closing this!