Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation

ThomasK33
Copy link
Member

@ThomasK33ThomasK33 commentedFeb 26, 2025
edited
Loading

Ensure that the version of Playwright installed with the Nix flake is equal to the one specified insite/package.json. -- This assertion ensures thatpnpm 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, aspnpm 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 Kosiewskitk@coder.com

johnstcn reacted with heart emoji
@ThomasK33Graphite App
Copy link
MemberAuthor

This stack of pull requests is managed byGraphite. Learn more aboutstacking.

@ThomasK33ThomasK33 marked this pull request as ready for reviewFebruary 26, 2025 15:29
@ThomasK33ThomasK33force-pushed thethomask33/02-26-fix_flake.nix_assert_equal_playwright_version_in_nix_and_package.json branch 2 times, most recently from810cd08 to2da521bCompareFebruary 26, 2025 15:35
Copy link
Member

@johnstcnjohnstcn 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 insite/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?

@ThomasK33Graphite App
Copy link
MemberAuthor

Are we affected by those regressions?

The alternative is to upgrade@playwright/test to1.50.1.

@johnstcn
Copy link
Member

Are we affected by those regressions?

The alternative is to upgrade@playwright/test to1.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 usepage.pause() in our tests.

@aslilac
Copy link
Member

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

@ThomasK33Graphite App
Copy link
MemberAuthor

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.

@ThomasK33Graphite App
Copy link
MemberAuthor

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 thatpnpm 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?

@ThomasK33ThomasK33force-pushed thethomask33/02-26-fix_flake.nix_assert_equal_playwright_version_in_nix_and_package.json branch from2da521b to860863bCompareFebruary 27, 2025 13:15
@aslilac
Copy link
Member

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

@ThomasK33Graphite App
Copy link
MemberAuthor

ThomasK33 commentedFeb 27, 2025
edited
Loading

not on workspace start for everyone

Maybe I misunderstand our startup script, but we explicitly callpnpm 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

oh nevermind! I didn't know we added that

Change-Id: I8cc78e842f7d0b1d2a90a4517a186a03636c5559Signed-off-by: Thomas Kosiewski <tk@coder.com>
@ThomasK33ThomasK33force-pushed thethomask33/02-26-fix_flake.nix_assert_equal_playwright_version_in_nix_and_package.json branch from860863b toab42451CompareFebruary 28, 2025 12:55
@ThomasK33Graphite App
Copy link
MemberAuthor

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 thepnpm 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-actionsgithub-actionsbot added the staleThis issue is like stale bread. labelMar 8, 2025
@ThomasK33ThomasK33 reopened thisMar 11, 2025
@johnstcnjohnstcn removed the staleThis issue is like stale bread. labelMar 11, 2025
@ThomasK33ThomasK33 changed the titlefix(flake.nix): assert equal playwright version in nix and package.jsonfix(flake.nix): synchronize playwright version in nix and package.jsonMar 11, 2025
@ThomasK33ThomasK33 merged commit9ded2cc intomainMar 11, 2025
66 checks passed
@ThomasK33ThomasK33 deleted the thomask33/02-26-fix_flake.nix_assert_equal_playwright_version_in_nix_and_package.json branchMarch 11, 2025 12:49
@github-actionsgithub-actionsbot locked and limited conversation to collaboratorsMar 11, 2025
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.
Reviewers

@johnstcnjohnstcnjohnstcn approved these changes

@mafredrimafredriAwaiting requested review from mafredri

@aslilacaslilacAwaiting requested review from aslilacaslilac is a code owner

Assignees

@ThomasK33ThomasK33

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

4 participants
@ThomasK33@johnstcn@mafredri@aslilac

[8]ページ先頭

©2009-2025 Movatter.jp