feat: per-package node version by KSXGitHub · Pull Request #8277 · pnpm/pnpm

KSXGitHub · 2024-07-04T16:59:09Z

TODO:

pnpm run
pnpm install, pnpm add, pnpm update
pnpm publish

zkochan · 2024-07-04T22:56:45Z

In the previous PR #6724 I have suggested to declare the config (useNodeVersion) in the package.json files instead of .npmrc. I think it is easier to implement this way. However, all the current options in the "pnpm" field of package.json are meant to be used in the root of the workspace only. cc @pnpm/collaborators

gluxon · 2024-07-04T23:22:00Z

Having this new setting be in per-project package.json files seems okay to me. I think it'll work as long as the docs are clear about the root pnpm key and per-project pnpm key.

KSXGitHub · 2024-07-05T09:03:40Z

Maybe we should add warnings as well for the use of root-only keys in local workspace package in another PR?

KSXGitHub · 2024-07-06T22:49:45Z

exec/plugin-commands-script-runners/test/runRecursive.ts

-// TODO: fix the related implementation then remove `.skip`
-test.skip('recursive run when some packages define node version, config does not define use-node-version', async () => {
-  preparePackages([
-    {
-      name: 'node-version-undefined',
-      scripts: {
-        'assert-node-version': `node -e "assert.equal(process.version, '${process.version}')"`,
-      },
-    },
-    {
-      name: 'node-version-18',
-      scripts: {
-        'assert-node-version': 'node -e "assert.equal(process.version, \'v18.0.0\')"',
-      },
-      pnpm: {
-        useNodeVersion: '18.0.0',
-      },
-    },
-    {
-      name: 'node-version-20',
-      scripts: {
-        'assert-node-version': 'node -e "assert.equal(process.version, \'v20.0.0\')"',
-      },
-      pnpm: {
-        useNodeVersion: '20.0.0',
-      },
-    },
-  ])
-  await run.handler({
-    ...DEFAULT_OPTS,
-    ...await filterPackagesFromDir(process.cwd(), [{ namePattern: '*' }]),
-    dir: process.cwd(),
-    recursive: true,
-    workspaceDir: process.cwd(),
-    workspaceConcurrency: 3, // if this step fails, change this value to 1 to see error message.
-  }, ['assert-node-version'])
-})


This test doesn't work when placed here but it work when moved to /pnpm/test. Not sure why.

zkochan · 2024-07-11T11:03:59Z

I am wondering about the location of this setting. We might want to move it to a subfield. Like pnpm.executionEnv.nodeVersion. In that case in the future we might add some more settings. Like env variables for instance.

KSXGitHub · 2024-07-11T18:49:21Z

@zkochan

I am wondering about the location of this setting. We might want to move it to a subfield. Like pnpm.executionEnv.nodeVersion. In that case in the future we might add some more settings. Like env variables for instance.

I have implemented this suggestion in c96153e. You may revert the change if you don't like it.

Aesthetically speaking, it makes certain part of the code more long-winded, but could it be fixed by reusing createPrepareExecutionEnv?

Regarding the "future", I wonder how would they live alongside nodeVersion within pnpm.executionEnv. Could it be pnpm.executionEnv.env like this?

{
  "pnpm": {
    "executionEnv": {
      "nodeVersion": "20.0.0",
      "env": {
        "USERNAME": "username",
        "TOKEN": "d94f9468de245385b5204d1554e248db"
      }
    }
  }
}

Or should this be a bit flatter (with intermediate executionEnv removed)?

zkochan · 2024-07-11T20:47:10Z

Regarding the "future", I wonder how would they live alongside nodeVersion within pnpm.executionEnv. Could it be pnpm.executionEnv.env like this?

I think it would be inside pnpm.executionEnv.variables.

Although, I am not sure it would make sense at all. Usually variables are either system specific or they contain secrets. Also, I am not sure you'd want to set the variables for all the scripts. Usually they are only needed for one specific command.

KSXGitHub

@zkochan What do you think about this?

KSXGitHub · 2024-07-12T10:32:10Z

exec/plugin-commands-script-runners/src/runRecursive.ts

+          if (pkg.package.manifest.pnpm?.executionEnv?.nodeVersion) {
+            lifecycleOpts.extraBinPaths = (await node.prepareExecutionEnv(opts, { executionEnv: pkg.package.manifest.pnpm.executionEnv })).extraBinPaths
+          }


Suggested change

if (pkg.package.manifest.pnpm?.executionEnv?.nodeVersion) {

lifecycleOpts.extraBinPaths = (await node.prepareExecutionEnv(opts, { executionEnv: pkg.package.manifest.pnpm.executionEnv })).extraBinPaths

}

const { executionEnv } = pkg.package.manifest.pnpm ?? {}

if (executionEnv) {

lifecycleOpts.extraBinPaths = (await node.prepareExecutionEnv(opts, { executionEnv })).extraBinPaths

}

KSXGitHub · 2024-07-12T10:33:22Z

exec/plugin-commands-script-runners/src/run.ts

+  if (manifest.pnpm?.executionEnv?.nodeVersion) {
+    lifecycleOpts.extraBinPaths = (await node.prepareExecutionEnv(opts, { executionEnv: manifest.pnpm.executionEnv })).extraBinPaths
+  }


Suggested change

if (manifest.pnpm?.executionEnv?.nodeVersion) {

lifecycleOpts.extraBinPaths = (await node.prepareExecutionEnv(opts, { executionEnv: manifest.pnpm.executionEnv })).extraBinPaths

}

const { executionEnv } = manifest.pnpm ?? {}

if (manifest.pnpm?.executionEnv?.nodeVersion) {

lifecycleOpts.extraBinPaths = (await node.prepareExecutionEnv(opts, { executionEnv })).extraBinPaths

}

zkochan · 2024-07-12T12:15:39Z

env/plugin-commands-env/src/replaceOrAddNodeIntoBinPaths.ts

+  } else {
+    return [...binPaths.slice(0, index), nodePath, ...binPaths.slice(index + 1)]
+  }
+}


I am not sure why this complex function is needed at all. The node location should be just appended to the array. Replacing is not good because we need the specified node path to have the highest priority. What if the replaced directory didn't have the highest priority? Also, even if it will be duplicated, I don't think it is an issue.

zkochan · 2024-07-12T13:37:09Z

env/plugin-commands-env/src/node.ts

+  }
+
+  return {
+    extraBinPaths: [await nodePathPromise, ...extraBinPaths ?? []],


it should be added to the end

it should be added to the end

It would allow extraBinPaths to override node. Do you still want this?

isn't that the whole point? To override node with the version specified in the setting.

It would allow the tail of extraBinPaths to override the node provided by the nodePathPromise. The tail of extraBinPaths may or may not include node from use-node-version of .npmrc. It would defeat the whole point.

Yes, you are correct. The directories at the beginning have bigger priority. It is correct as it is now.

I have added a test that would fail should extraBinPaths was handled incorrectly (leading to use-node-version overrides executionEnv): 2721d44

You may revert if you find it unnecessary.

KSXGitHub · 2024-07-13T00:54:12Z

.changeset/yellow-brooms-sort.md

+"@pnpm/plugin-commands-installation": major
+"@pnpm/plugin-commands-publishing": major
+"@pnpm/plugin-commands-script-runners": major
+"@pnpm/plugin-commands-env": minor


@zkochan Since 7e7c412 breaks the API,

Suggested change

"@pnpm/plugin-commands-env": minor

"@pnpm/plugin-commands-env": major

antitoxic · 2024-08-14T08:53:53Z

OMG! amazing! Thank you @KSXGitHub @zkochan ❤️

PS: Does that also execute the post.... hooks like postinstall using the specific node version?

zkochan · 2024-08-26T13:35:50Z

No, this is somewhat more complex as the dependency may be used by several different projects in the workspace, which all use different node.js versions. So, which node.js version should be used to build the dependency?

We can add a field for the dependency itself to tell which node.js version it requires for being build. See related RFC: pnpm/rfcs#7

KSXGitHub added 6 commits July 4, 2024 23:56

feat: createBinPathsWithNodeVersion

4a72a94

refactor: use it in main.ts

9e996c0

feat: use it in run.ts and runRecursive.ts

c003153

fix: findIndex

7d6d03f

refactor: move resolving of baseDir

c9c59d6

test: replaceOrAddNodeIntoBinPaths

6e63a7a

KSXGitHub added 12 commits July 5, 2024 16:16

fix: type errors in tests

77474ff

chore(deps): update @pnpm/registry-mock

38fd7c3

test: prepare a test

37b5670

chore(git): merge from main

7e3c4ae

fix: typescript

5a22241

test: run with[out] node version

f403ad0

test: change script name

b3d626c

style: fix eslint

68f3d65

test: recursive run

c452d27

test: move it to where it works

b2ea3b5

test: when config has node version

34b8ba4

test: change wordings

25b47c4

KSXGitHub commented Jul 6, 2024

View reviewed changes

KSXGitHub added 8 commits July 8, 2024 21:02

test: remove test that no longer applies

477f6d2

feat: make install scripts use pnpm.useNodeVersion

0c6edc4

test: lifecycle scripts during recursive install

bc1bd02

fix: clone binPaths to avoid modification

2fd041c

test: remove the assertion that doesn't apply

4f8ae76

feat(publish): pnpm.useNodeVersion

c998d06

chore(git): merge from main

2fbb51c

refactor: fix eslint

ce46d01

feat: pnpm.executionEnv.nodeVersion

c96153e

zkochan and others added 5 commits July 12, 2024 02:26

test: fix

2a69496

refactor: prepare exec env

99fff37

test: fix

9dc3930

test: fix

f8de022

test: unskip and fix

3dc44bd

KSXGitHub commented Jul 12, 2024

View reviewed changes

refactor: prepare exec env

278e78d

zkochan requested changes Jul 12, 2024

View reviewed changes

KSXGitHub added 2 commits July 12, 2024 20:16

refactor: just prepend

0d8cb5f

refactor: remove unused interface

2768e48

KSXGitHub requested a review from zkochan July 12, 2024 13:19

zkochan reviewed Jul 12, 2024

View reviewed changes

KSXGitHub added 2 commits July 12, 2024 21:22

test: prevent incorrect bin path manipulation

2721d44

test: merge similar tests

30cd0ad

KSXGitHub requested a review from zkochan July 12, 2024 14:56

zkochan added 2 commits July 13, 2024 02:16

refactor: remove getNodeBinDir from dlx

b6c2d20

refactor: prepare exec env

7e7c412

zkochan approved these changes Jul 13, 2024

View reviewed changes

test: fix

720ed1a

KSXGitHub commented Jul 13, 2024

View reviewed changes

zkochan merged commit 0ef168b into main Jul 13, 2024

zkochan deleted the per-pkg-node-version-6720 branch July 13, 2024 12:08

oyvinmar mentioned this pull request Aug 6, 2024

pnpm 9.6.0 - Command not found #8359

Closed

4 tasks

antitoxic mentioned this pull request Aug 28, 2024

Execution Environments pnpm/rfcs#7

Closed

2 tasks

-          if (pkg.package.manifest.pnpm?.executionEnv?.nodeVersion) {
-            lifecycleOpts.extraBinPaths = (await node.prepareExecutionEnv(opts, { executionEnv: pkg.package.manifest.pnpm.executionEnv })).extraBinPaths
-          }
+          const { executionEnv } = pkg.package.manifest.pnpm ?? {}
+          if (executionEnv) {
+            lifecycleOpts.extraBinPaths = (await node.prepareExecutionEnv(opts, { executionEnv })).extraBinPaths
+          }

	"@pnpm/plugin-commands-env": minor
	"@pnpm/plugin-commands-env": major

Uh oh!

Conversation

KSXGitHub commented Jul 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zkochan commented Jul 4, 2024

Uh oh!

gluxon commented Jul 4, 2024

Uh oh!

KSXGitHub commented Jul 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zkochan commented Jul 11, 2024

Uh oh!

KSXGitHub commented Jul 11, 2024

Uh oh!

zkochan commented Jul 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KSXGitHub left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KSXGitHub Jul 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

antitoxic commented Aug 14, 2024

Uh oh!

zkochan commented Aug 26, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

KSXGitHub commented Jul 4, 2024 •

edited

Loading

KSXGitHub commented Jul 5, 2024 •

edited

Loading

zkochan commented Jul 11, 2024 •

edited

Loading

KSXGitHub Jul 13, 2024 •

edited

Loading