-
Notifications
You must be signed in to change notification settings - Fork 16
feat: add ability to get playwright version from lock file #1112
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems overly complicated. Looking at https://github.com/microsoft/playwright/blob/a6cb0093567e3d539f93978362a8fa784c73f2fe/packages/playwright-test/package.json, @playwright/test
exports its own package.json
. Is the following not enough?
const { version } = require('@playwright/test/package.json')
Or am I misunderstanding what exactly you need from there?
@sorccu We are trying to get it from the lock file, so the exact version the user is using. |
Do you mean exact version ( |
@sorccu I mean exact version |
That's exactly what you get with the snippet I posted. Though it has one caveat: it only works correctly as long as the CLI itself does not have Alternatively you could look up the closest
|
const packageJson = JSON.parse(await readFile(modulePath, 'utf-8')); | ||
return normalizeVersion(packageJson.version); | ||
} catch { | ||
// If node_modules not found, fall back to checking the project's package.json |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reason for the fallback? Like, did you actually encounter this issue, and if so, under what conditions (did they not run npm install?), or are you just adding this for good measure?
Since you have to run npm install (or equivalent) to even have access to the checkly CLI in a functional way, I'm a bit confused why this is needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you use Yarn PnP you will not generate node_modules
unless told https://yarnpkg.com/features/pnp#how-does-it-work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, ok. That could be solved by just using require or await import though, because then it would go through the normal resolution system. Or, at the very least require.resolve
should help if you insist on loading the file manually.
Do you have a reason to not use require/import? Does it make testing harder or...?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't able to make testing work for require.resolve
to resolve to a specific path (it was using default cli playwright version path) for some reason.
Affected Components
Notes for the Reviewer
Add ability to parse lock files and get the playwright version from them so we can populate it as part of the playwright check