Skip to content

fix(flake.nix): synchronize playwright version in nix and package.json #16715

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

Conversation

ThomasK33
Copy link
Member

@ThomasK33 ThomasK33 commented Feb 26, 2025

Ensure that the version of Playwright installed with the Nix flake is equal to the one specified in site/package.json. -- This assertion ensures that pnpm playwright:install will not attempt to download newer browser versions not present in the Nix image, fixing the startup script and reducing the startup time, as pnpm playwright:install will not download or install anything.

We also pre-install the required Playwright web browsers in the dogfood Dockerfile. This change prevents us from redownloading system dependencies and Google Chrome each time a workspace starts.

Change-Id: I8cc78e842f7d0b1d2a90a4517a186a03636c5559
Signed-off-by: Thomas Kosiewski tk@coder.com

Copy link
Member Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

@ThomasK33 ThomasK33 requested a review from johnstcn February 26, 2025 15:29
@ThomasK33 ThomasK33 marked this pull request as ready for review February 26, 2025 15:29
@ThomasK33 ThomasK33 force-pushed the thomask33/02-26-fix_flake.nix_assert_equal_playwright_version_in_nix_and_package.json branch 2 times, most recently from 810cd08 to 2da521b Compare February 26, 2025 15:35
@johnstcn johnstcn requested review from mafredri and aslilac February 26, 2025 15:38
Copy link
Member

@johnstcn johnstcn left a comment

Choose a reason for hiding this comment

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

Downgrading the version of playwright in site/package.json means we're missing the fixes for four regressions fixed in v1.47.1 and v1.47.2:

Is there any other alternative to this approach?

Copy link
Member Author

Are we affected by those regressions?

The alternative is to upgrade @playwright/test to 1.50.1.

@johnstcn
Copy link
Member

Are we affected by those regressions?

The alternative is to upgrade @playwright/test to 1.50.1.

I'm not entirely sure. @aslilac @mafredri since you were the last two folks to bump the playwright version, can you weigh in here?

@mafredri
Copy link
Member

I simply updated to the latest one at the time, so no deeper intention. In general I'd prefer upgrade over downgrade, but no idea what changes have gone in. Regressions from going .2 to .0 don't seem relevant IMO but don't know if we use page.pause() in our tests.

@aslilac
Copy link
Member

aslilac commented Feb 26, 2025

I don't know much about nix, but I really don't like the idea of not being able to use newer versions of a package from an entirely different package manager just because nix says so

Copy link
Member Author

This PR focuses not on locking down the Playwright version but ensuring that the browsers required by a specific Playwright version are already pre-installed in the image. If the browsers are not pre-installed, Playwright attempts to download them from the internet, which can negatively impact startup times, bandwidth usage, and reproducibility.

I am OK with upgrading to a newer Playwright version, but the constraint of pre-installing the browsers into the image is firm.

@johnstcn
Copy link
Member

This PR focuses not on locking down the Playwright version but ensuring that the browsers required by a specific Playwright version are already pre-installed in the image. If the browsers are not pre-installed, Playwright attempts to download them from the internet, which can negatively impact startup times, bandwidth usage, and reproducibility.

I am OK with upgrading to a newer Playwright version, but the constraint of pre-installing the browsers into the image is firm.

So this constraint should only affect users of the Nix image? IMO it's a reasonable constraint.

Copy link
Member Author

Yes, it causes the Nix image build to exit early if the Nix Playwright version does not match the one specified in package.json, as it cannot guarantee that pnpm playwright:install will be a no-op.

Interestingly, I couldn't find us pre-caching or downloading the browsers in the standard dogfood image, which leads me to conclude that we are re-downloading Google Chrome and friends each time a workspace starts?

@ThomasK33 ThomasK33 force-pushed the thomask33/02-26-fix_flake.nix_assert_equal_playwright_version_in_nix_and_package.json branch from 2da521b to 860863b Compare February 27, 2025 13:15
@aslilac
Copy link
Member

aslilac commented Feb 27, 2025

does it only affect nix users tho? it places a constraint on what version of playwright we can place in package.json, which affects everyone

which leads me to conclude that we are re-downloading Google Chrome and friends each time a workspace starts?

not on workspace start for everyone, you either have to remember to download them before running the e2e tests or put the download command in your ~/personalize script

Copy link
Member Author

ThomasK33 commented Feb 27, 2025

not on workspace start for everyone

Maybe I misunderstand our startup script, but we explicitly call pnpm playwright:install as part of it, or am I understanding something wrong?

cd "${local.repo_dir}/site" && pnpm install && pnpm playwright:install

@aslilac
Copy link
Member

aslilac commented Feb 27, 2025

oh nevermind! I didn't know we added that

Change-Id: I8cc78e842f7d0b1d2a90a4517a186a03636c5559
Signed-off-by: Thomas Kosiewski <tk@coder.com>
@ThomasK33 ThomasK33 force-pushed the thomask33/02-26-fix_flake.nix_assert_equal_playwright_version_in_nix_and_package.json branch from 860863b to ab42451 Compare February 28, 2025 12:55
Copy link
Member Author

Yeah, I just double-checked, and it indeed reinstalls system dependencies and Chromium at each workspace start due to that startup script.

I've refactored this behavior by removing the pnpm playwright:install from the startup script. We're pre-installing the required system dependencies and browser versions in the Dogfood Docker image, which should also significantly increase our startup times for nix and non-nix workspaces.

This also means we're pinning the Playwright version in the Dockerfile, as we cannot access the site's package.json at build time.

@github-actions github-actions bot added the stale This issue is like stale bread. label Mar 8, 2025
@github-actions github-actions bot closed this Mar 11, 2025
@ThomasK33 ThomasK33 reopened this Mar 11, 2025
@ThomasK33 ThomasK33 requested a review from johnstcn March 11, 2025 05:17
@johnstcn johnstcn removed the stale This issue is like stale bread. label Mar 11, 2025
@ThomasK33 ThomasK33 changed the title fix(flake.nix): assert equal playwright version in nix and package.json fix(flake.nix): synchronize playwright version in nix and package.json Mar 11, 2025
@ThomasK33 ThomasK33 merged commit 9ded2cc into main Mar 11, 2025
66 checks passed
@ThomasK33 ThomasK33 deleted the thomask33/02-26-fix_flake.nix_assert_equal_playwright_version_in_nix_and_package.json branch March 11, 2025 12:49
@github-actions github-actions bot locked and limited conversation to collaborators Mar 11, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants