Throw error when multiple versions specified #122
Conversation
src/install-pnpm/run.ts
Outdated
| if (GITHUB_WORKSPACE) { | ||
| ({ packageManager } = JSON.parse(await readFile(path.join(GITHUB_WORKSPACE, packageJsonFile), 'utf8'))) | ||
| } |
There was a problem hiding this comment.
If there is no package.json at root, this will result in error. Please fix this.
There was a problem hiding this comment.
Hm, this error was also there before, wasn't it? Should that be a separate PR?
Happy to wrap it in a try/catch to swallow the error if you think it belongs in this PR
There was a problem hiding this comment.
Hm, this error was also there before, wasn't it? Should that be a separate PR?
Before, if user already specifies version in the action, no error would actually happen.
Happy to wrap it in a
try/catchto swallow the error if you think it belongs in this PR
If the file doesn't exist (error.code === 'ENOENT'), assign undefined.
There was a problem hiding this comment.
packageManager is already set to undefined
I added a condition to swallow the error if error.code === 'ENOENT':
src/install-pnpm/run.ts
Outdated
| if (version) { | ||
| if (typeof packageManager === 'string') { | ||
| throw new Error(`Multiple versions of pnpm specified: | ||
| - version ${version} in the GitHub Action config with the key "version" | ||
| - version ${packageManager} in the package.json with the key "packageManager" | ||
| Remove one of these versions to avoid version mismatch errors like ERR_PNPM_BAD_PM_VERSION`) | ||
| } |
There was a problem hiding this comment.
but why should it fail if the versions match?
There was a problem hiding this comment.
Added a condition to skip if versions match:
|
@KSXGitHub @zkochan is there anything left to resolve here? Can this be merged? |
|
I guess this is a breaking change |
|
Thanks for the review, the commit improving the robustness of the error condition and merge! I would also agree that this is a breaking change - I will keep an eye on the releases for |
| if (version) { | ||
| if ( | ||
| typeof packageManager === 'string' && | ||
| packageManager.replace('pnpm@', '') !== version |
There was a problem hiding this comment.
this isn't enough, there can be a hash at end. see recent corepack release.
Followup to #33 and #35 by @Jack-Works
Throws error if multiple versions of pnpm are specified, both in:
This will avoid
ERR_PNPM_BAD_PM_VERSIONerrors as documented here:run: corepack enable? #105 (comment)cc @KSXGitHub