garris / BackstopJS

Catch CSS curve balls.
http://backstopjs.org
MIT License
6.67k stars 605 forks source link

build (engines)!: Update dependencies for engines #1471

Closed fuhlig closed 1 year ago

fuhlig commented 1 year ago

BREAKING CHANGE: run npm install

tkrah commented 1 year ago

You should consider that the docker image does only support chromium up-to version:

108.0.5359.94-1~deb11u1

I am not an expert here if puppeteer with chromium now on 112 will function for sure - maybe use a puppeteer version which supports only up to 108 and does not need a version greater than that to be on the safe side, we can't use the puppeteer included versions because they don't have multi-arch support (speaking linux/arm64).

Looking at https://github.com/puppeteer/puppeteer/blob/main/versions.js we should use Puppeteer: v19.2.0 @fuhlig .

tkrah commented 1 year ago

We could use the chromium from the playwright install which is 112.0.5615.29 - but I am unsure how to know the path for that installation, because it has a version component in it and from the playwright docs I don't see a way to specify a concrete version, so that we can have a stable working build and set the puppeteer variable to the right path. Ideas anyone with more knowledge of playwright? Until this is evaulated we should use the debian one though with the correct puppeteer version above.

fuhlig commented 1 year ago

Could you elaborate on the compat issues?

Running backstop with puppeteer@19.8.2 (Chrome 112) works when running with --docker.

When building a Docker image with node:16-bullseye (which could be updated to v18 LTS) and running apt-get install chromium I get

Chromium 111.0.5563.110 built on Debian 11.6, running on Debian 11.6
tkrah commented 1 year ago

It may work, but it is not guaranteed and issues are likely according to puppeteers dev people, look at their docs. So better be safe here and use a version which does match, 19.7.0 if 111 is included.

https://packages.debian.org/search?suite=bullseye&searchon=names&keywords=Chromium does only result in 108.x ... Interesting where that 111 is coming from.

Edit: https://security-tracker.debian.org/tracker/source-package/chromium

From there via the security channel. In that case please match that 111 with 19.7.0.

@node18 - garris did not tell anything on the last patches to that file about that base image version, that's why I thought he wants to stay on 16 at the moment.

fuhlig commented 1 year ago

@tkrah Thanks for checking out and clarifying.

garris commented 1 year ago

Thank you @fuhlig and @tkrah!

fuhlig commented 1 year ago

Cheers for the quick responses and merge! 🚀

When is the next npm release planned?

garris commented 1 year ago

Hi. I'm still traveling. Will be back Saturday -- will try to do it on the weekend if I can get a few mins.

Cheers

garris commented 1 year ago

@tkrah @fuhlig BackstopJS v6.2.1 is released to NPM & Docker. Thanks for your patience. Cheers.

genepaul commented 1 year ago

I'm confused. This change was marked as a "breaking change," and yet it seems to have been released as a patch. Is it not actually breaking? I've been noticing changes in my visual regression tests that I'm trying to track down, and wondering if getting a new version of puppeteer (4 MAJOR versions later, no less) could be causing it.

tkrah commented 1 year ago

Puppeteer versions must match certain chrome versions, as the base image demands a certain chrome version we have to migrate anyway, no matter if it breaks or not if we want to ensure that puppeteer is compatible (according to their docs) - so it was already a "bug" in backstopjs to not match those demands in the past - so fixing bugs even if they break things is ok for patch versions. @genepaul - so yes your changes may be coming from there (you can easily test it by reverting that change and rebuild backstopjs) - but you have to cope with the changes anyway - you can stay on an older version if you can't upgrade yet.

genepaul commented 1 year ago

@tkrah you and I have different understandings of how semantic versioning works. A "fix" that breaks things is still a breaking change, thus a major version, which would allow consumers to opt in to the fix. Releasing as a patch forces those who may have used semver ranges to pull changes in without realizing it.

tkrah commented 1 year ago

You are right, but no where did I found in the docs of BackstopJS that it does follow semantic versioning, did you? So I would not expect it to follow those rules if the author does not told us to do so. @genepaul - that's why I was under the impression that fixing bugs even if they break stuff is ok ;-). In the end the author did merge it - so it confirms that we don't follow those rules here ;-).

genepaul commented 1 year ago

lol ok. I guess I thought setting the version numbers to \<major>.\<minor>.\<patch> was an implicit indication that this library followed a well-known practice in the open source community. My bad for making such an assumption.