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

fix: correctly detect the active Node js version during headless installation #7801

Merged
merged 3 commits into from Mar 17, 2024

Conversation

dy-dx
Copy link
Contributor

@dy-dx dy-dx commented Mar 16, 2024

This is a proof of concept for fixing #5266.

When pnpm is bundled with node.js (via standalone installer), and engine-strict=true, pnpm falls back to using the bundled node.js.

pnpm properly detects the system node.js version when installing new packages, but not when doing headless installs (i.e. when the lockfile is up to date).

I think the issue is the extendOptions function:

const opts = extendOptions(maybeOpts)

It sets options.nodeVersion = process.version, i.e. whatever node is bundled with pnpm.

This doesn't seem to matter during the code path for non-headless installations.
But during headless installations, it prevents pnpm from checking the system Node.js version when checking package compatibility.

@dy-dx dy-dx requested a review from zkochan as a code owner March 16, 2024 00:46
Copy link

welcome bot commented Mar 16, 2024

💖 Thanks for opening this pull request! 💖
Please be patient and we will get back to you as soon as we can.

@zkochan
Copy link
Member

zkochan commented Mar 16, 2024

But the same code is present here:

node: options.nodeVersion ?? getSystemNodeVersion(),

So does this change actually make any difference?

So you just need to replace process.version with opts.nodeVersion. No need to run getSystemNodeVersion

But we also need to check where and how is opts.nodeVersion set.

@@ -187,7 +188,7 @@ const defaults = (opts: InstallOptions) => {
},
lockfileDir: opts.lockfileDir ?? opts.dir ?? process.cwd(),
lockfileOnly: false,
nodeVersion: process.version,
nodeVersion: opts.nodeVersion ?? getSystemNodeVersion(),
Copy link
Member

Choose a reason for hiding this comment

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

This should be enough

Suggested change
nodeVersion: opts.nodeVersion ?? getSystemNodeVersion(),
nodeVersion: opts.nodeVersion,

Copy link
Member

Choose a reason for hiding this comment

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

The issue is that we pass process.version. getSystemNodeVersion will be used anyway inside package-is-installable.

@zkochan zkochan merged commit 0fa26f4 into pnpm:main Mar 17, 2024
8 checks passed
Copy link

welcome bot commented Mar 17, 2024

Congrats on merging your first pull request! 🎉🎉🎉

@dy-dx
Copy link
Contributor Author

dy-dx commented Mar 17, 2024

So you just need to replace process.version with opts.nodeVersion. No need to run getSystemNodeVersion

@zkochan yes I agree, I actually tried that first and I can confirm that it also solves the issue. But I wasn't sure if the resulting typing changes were going to be problematic or not.

Thanks for the quick response!

zkochan added a commit that referenced this pull request Mar 17, 2024
…allation (#7801)

close #5266

---------

Co-authored-by: Zoltan Kochan <z@kochan.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants