Skip to content
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

✨ Expose used chrome versions #279

Merged
merged 3 commits into from
Apr 6, 2021

Conversation

olebedev
Copy link
Contributor

@olebedev olebedev commented Mar 31, 2021

Background

The @percy/core package manages chromium version on its own when no executables provided, so there is no versions mismatch possible in this scenario. However, when the PERCY_BROWSER_EXECUTABLE env var is set to point to a chromium executable it becomes difficult to manage the chromium version and keep it aligned with the ones that the library is supposed to work with. Especially, when the dependency definition in package.json file is set like:

{
  ...
  "peerDependencies": {
    "@percy/core": "*",
  },
  ...
}

In this PR

Expose chromium versions to be able to assert that the local one fits the requirement. For example:

const { chromium } = require('@percy/core/dist/install');
assert.equal(
  '812847',
  chromium.revisions.linux,
  'The local chromium version for @percy/core don\'t match the expected one!',
);

By exposing the chromium version, we can provide a guarantee that the versions are matching along with the ability to upgrade the library independently while the local and required versions are the same.

cc @joscha

@wwilsman
Copy link
Contributor

wwilsman commented Apr 1, 2021

Hey @olebedev! Thanks for the PR!

Only small suggestion would be to set this property as installChromium.revisions since these revisions only pertain to this particular function. Then you can remove the additional export and access revisions as install.chromium.revisions.

@olebedev
Copy link
Contributor Author

olebedev commented Apr 6, 2021

Hi @wwilsman, done in d6e879e, please have a look!

Copy link
Contributor

@wwilsman wwilsman left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!

Our CI events weren't set up properly for forks at the time you forked, so I'll just go ahead and merge this anyway with pending checks since I don't foresee any issues from this change. 😄

@wwilsman wwilsman changed the title expose used chrome versions ✨ Expose used chrome versions Apr 6, 2021
@wwilsman wwilsman merged commit 9400c51 into percy:master Apr 6, 2021
@joscha
Copy link

joscha commented Apr 6, 2021

Thanks @wwilsman! Can you ping here when this goes into a new version, please?

@Robdel12 Robdel12 added the ✨ enhancement New feature or request label Apr 8, 2021
@Robdel12
Copy link
Contributor

Robdel12 commented Apr 8, 2021

Boom went out in 1.0.0-beta.47 (https://github.com/percy/cli/releases/tag/v1.0.0-beta.47) @joscha / @olebedev :D thanks for the PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✨ enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants