-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
reject mismatched corepack and detected package managers #11603
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 3b2e24f The changes in this PR will be included in the next version bump. This PR includes changesets to release 7 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
}; | ||
case 'pnpm 6': | ||
default: | ||
return no_override; | ||
return undefined; |
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.
Question: Do we want to be more specific here with what was detected, but maybe leave the path
blank because it matches the default? This would make the comparison between detected package managers and versions to corepack specified ones more complete.
Right now, detecting pnpm@6.x
(for example) would come back as nothing, not comparing to what was specified by corepack.
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'm in favor of being more specific in what we return.
throw new Error( | ||
`Detected package manager (by lockfile) "${detectedPackageManger.detectedLockfile}" does not match intended corepack package manager "${corepackPackageManager}". Update your lockfile or "package.json#packageManager" values to match.` | ||
); | ||
} |
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.
Do we have a test case for this throw?
…thub.com/vercel/vercel into endangeredmassa/reject-mismatch-corepack
switch (cliType) { | ||
case 'npm': | ||
switch (true) { | ||
case corepackEnabled: | ||
return no_override; | ||
case shouldUseNpm7(lockfileVersion, nodeVersion): |
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 think we need to delete this function. If the data is flowing properly, it looks like it always returns false. I'd check the PR that introduced this logic to be sure we're considering the full context.
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 think we should also fix the build container. It looks like it's not passing in node version (at least some of the time) into the exported runNpmInstall
function. Make sure it's passing that in every time it calls that function.
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.
Nothing of note in the PR: #6553
}; | ||
case 'pnpm 8': | ||
// pnpm 8 | ||
return { | ||
path: '/pnpm8/node_modules/.bin', | ||
detectedLockfile: 'pnpm-lock.yaml', | ||
detectedPackageManager: 'pnpm 8', | ||
detectedPackageManager: 'pnpm@8.x', |
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.
We need to broaden the detection here. We know that (in this case for example, but this applies to all cases) the lockfile was generated by pnpm@8
, but it could have been read by pnpm@8
or pnpm@9
. We need to only fail the build if you try to use some other pnpm
version.
Table of supported values: pnpm/spec#7
f22267c
to
fd27e8c
Compare
New and removed dependencies detected. Learn more about Socket for GitHub ↗︎
🚮 Removed packages: npm/@vercel-internals/types@1.0.37, npm/@vercel/client@13.2.8, npm/@vercel/gatsby-plugin-vercel-builder@2.0.32, npm/@vercel/next@4.2.15, npm/@vercel/static-build@2.5.10 |
fd27e8c
to
f22267c
Compare
When corepack is used and the package manager or version does not match the inferred package manager or version from the lockfile, we should throw an error. Right now, this can happen with
pnpm
(for example) wherepnpm
only logs a warning, causing indeterministic builds.This PR mostly updates
getPathOverrideForPackageManager
to not care about corepack, allowing the calling code to compare the detected values to the corepack values.