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.
There was a problem hiding this comment.
Oh sorry for any breakage! Can you provide an example of the hash + delimiter? I can open a new PR.
There was a problem hiding this comment.
I saw this behavior in Corepack just today, the packageManager specifier looks like this:
"packageManager": "[email protected]+sha512.14e915759c11f77eac07faba4d019c193ec8637229e62ec99eefb7cf3c3b75c64447882b7c485142451ee3a6b408059cdfb7b7fa0341b975f12d0f7629c71195"Looks like it was released in [email protected] in the following PR:
[email protected]https://github.com/nodejs/corepack/releases/tag/v0.26.0- PR adding
"packageManager"field topackage.jsonfeat: Pins the package manager as it's used for the first time nodejs/corepack#413
There was a problem hiding this comment.
@viceice but maybe this is actually desirable behavior, since:
- the
versioninput onpnpm/action-setupsupports a specifier with a Corepack hash (pnpm/action-setupusespnpm.cjs install, see output ofpnpm installbelow) - technically it could be argued that
9.1.1is not the same as9.1.1+sha512.14e915759c11f77eac07faba4d019c193ec8637229e62ec99eefb7cf3c3b75c64447882b7c485142451ee3a6b408059cdfb7b7fa0341b975f12d0f7629c71195, so stripping the hash is maybe not wanted
$ pnpm install [email protected]+sha512.14e915759c11f77eac07faba4d019c193ec8637229e62ec99eefb7cf3c3b75c64447882b7c485142451ee3a6b408059cdfb7b7fa0341b975f12d0f7629c71195
Packages: +1
+
Progress: resolved 1, reused 0, downloaded 1, added 1, done
dependencies:
+ pnpm 9.1.1
Done in 1.3sThere was a problem hiding this comment.
Thanks, but does it also work with the standalone version?
There was a problem hiding this comment.
Looks like it, yep:
$ pnpm install @pnpm/[email protected]+sha512.14e915759c11f77eac07faba4d019c193ec8637229e62ec99eefb7cf3c3b75c64447882b7c485142451ee3a6b408059cdfb7b7fa0341b975f12d0f7629c71195
Packages: +2
++
Downloading @pnpm/[email protected]: 22.84 MB/22.84 MB, done
Progress: resolved 7, reused 1, downloaded 2, added 2, done
node_modules/.pnpm/@[email protected]/node_modules/@pnpm/exe: Running preinstall script, done in 46ms
dependencies:
+ @pnpm/exe 9.1.1
Done in 8.6s
* Throw error when multiple versions specified * fix: fmt * fix: fmt * Swallow error on ENOENT * Match versions * refactor: install pnpm --------- Co-authored-by: Khải <[email protected]> Co-authored-by: Zoltan Kochan <[email protected]>
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