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

Execution Environments #7

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Execution Environments #7

wants to merge 5 commits into from

Conversation

zkochan
Copy link
Member

@zkochan zkochan commented Aug 26, 2024

TODO:

  • What if the consumer of the package wants to use a specific node.js version?
  • also mention how it interacts with nodeVersion. If both nodeVersion and jsRuntime is define, is it going to error or is one going to be prioritized over the other?

@zkochan zkochan changed the title Execution Environment Execution Environments Aug 26, 2024
Comment on lines 59 to 75
If we don't want to control the execution env of the published package, set the optional `localOnly` setting to `true`. For instance:

```json
{
"name": "cowsay",
"version": "1.0.0",
"bin": "bin.js",
"pnpm": {
"executionEnv": {
"js": "[email protected]",
"localOnly": true
}
}
}
```

In this case, pnpm will remove the `executionEnv` setting from the `package.json` file on publish and the binary of the package will be executed with whatever runtime will be installed globally on the target machine.

Choose a reason for hiding this comment

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

Doesn't pnpm already remove the "pnpm" field from package.json before publishing a package?

Copy link
Member Author

Choose a reason for hiding this comment

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

It does remove the field currently. But this new subfield should not be removed.

Choose a reason for hiding this comment

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

Effectively, you are creating a pnpm-specific extension for the whole JS packages ecosystem.

Copy link
Member Author

Choose a reason for hiding this comment

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

Those packages will work with other package managers just fine. But with pnpm they will be more stable as the runtime will be locked.

@KSXGitHub
Copy link

js feels too vague. I think runtime or jsRuntime would describe its purpose better.

@zkochan
Copy link
Member Author

zkochan commented Aug 26, 2024

jsRuntime sounds good to me

@KSXGitHub
Copy link

You should also mention how it interacts with nodeVersion. If both nodeVersion and jsRuntime is define, is it going to error or is one going to be prioritized over the other?

@antitoxic
Copy link

antitoxic commented Aug 26, 2024

Thank you about thinking on this 🙂

  1. At the company I'm at right now, we are already using pnpm.executionEnv.nodeVersion. I'm assuming this will be replacing this setting?
  2. Are we using package.json for this setting instead of local package-specific .npmrc because other tools do something similar (e.g. corepack)? Or is there another reason? Since pnpm already uses .npmrc for its settings, so effectively now there are 2 places for having pnpm-specific settings. I'm still totally grateful they exist, just wondering if that's on purpose or not.

@zkochan
Copy link
Member Author

zkochan commented Aug 26, 2024

We should explore one more scenario. What if the consumer of the package wants to use a specific node.js version? They should be able to tell pnpm to run a specific dependency's postinstall script with a given node.js version and to run a specific CLI with the given node.js version.

Also, how will we cleanup unused Node.js versions from the global cache? We currently have the pnpm env rm command for removing Node.js from the global cache and we don't check if that node.js version is used by anything.

Also, if we will support other js runtimes, should we change the pnpm env command, which currently hardcodes node.js?

At the company I'm at right now, we are already using pnpm.executionEnv.nodeVersion. I'm assuming this will be replacing this setting?

Yes, I think we will replace it with this more powerful alternative.

Are we using package.json for this setting instead of local package-specific .npmrc because other tools do something similar (e.g. corepack)? Or is there another reason? Since pnpm already uses .npmrc for its settings, so effectively now there are 2 places for having pnpm-specific settings. I'm still totally grateful they exist, just wondering if that's on purpose or not.

I agree that it is confusing. Some settings are in package.json, some in .npmrc. In this specific case it makes sense to add the setting to package.json as we want to use it, when the package gets installed as a dependency.

text/0003-execution-environment.md Outdated Show resolved Hide resolved

This RFC introduces settings for controlling what execution environment (Node.js, Bun, Deno) will be used for a package during:

* runnings its lifecycle scripts

Choose a reason for hiding this comment

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

One thing to note is that lifecycle scripts can technically choose to execute node, deno, and bun in the same command because they are shell scripts.

Copy link
Member Author

@zkochan zkochan Aug 26, 2024

Choose a reason for hiding this comment

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

yes, you are right. So, should we support specifying all of them?

Copy link

@KSXGitHub KSXGitHub Aug 26, 2024

Choose a reason for hiding this comment

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

There should still be a primary runtime for installing and running CLI app.

Lifecycles OTOH can take advantage of executionEnv.nodeVersion, executionEnv.denoVersion, executionEnv.bunVersion.

I don't quite understand the building item, is it lifecycle?

Copy link
Member Author

Choose a reason for hiding this comment

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

The building part is when the package is installed as a dependency. If it has a "postinstall" script, it will be executed to build the package. Or if it has a binding.gyp file, then node-gyp will run to build the package (it can still run node under the hood).

Copy link
Member Author

Choose a reason for hiding this comment

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

Lifecycles OTOH can take advantage of executionEnv.nodeVersion, executionEnv.denoVersion, executionEnv.bunVersion.

ok, so you suggest to keep the nodeVersion field, add [runtime]Version fields and a jsRuntime field. In that case, I guess jsRuntime will always be used, when the package is installed as a dependency (so the localOnly field is not needed).

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member Author

@zkochan zkochan Aug 26, 2024

Choose a reason for hiding this comment

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

In the referenced issue they have also pointed out that there is an existing field for specifying runtime environments: engines.

Copy link
Member Author

Choose a reason for hiding this comment

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

I am fine with your suggestion to have nodeVersion, denoVersion, bunVersion. I am not sure about the rest of the suggestion though. Especially as having a cliRuntime should be optional, so automatically generating it doesn't makes sense.

Something like an object with setting could work too:

{
  "pnpm": {
    "executionEnv": {
      "nodeRuntime": {
        "version": "20.16.0",
        "cli": true
      }
    }
  }
}

Choose a reason for hiding this comment

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

I am fine with your suggestion to have nodeVersion, denoVersion, bunVersion. I am not sure about the rest of the suggestion though. Especially as having a cliRuntime should be optional, so automatically generating it doesn't makes sense.

Something like an object with setting could work too:

{
  "pnpm": {
    "executionEnv": {
      "nodeRuntime": {
        "version": "20.16.0",
        "cli": true
      }
    }
  }
}

I'm not sure about adding another nesting level. Besides that, this also creates an invalid state where nodeRuntime.cli, denoRuntime.cli, and bunRuntime.cli are all defined, compared to cliRuntime which doesn't have invalid state.

Especially as having a cliRuntime should be optional, so automatically generating it doesn't makes sense.

We can improve it a bit: cliRuntime is only required when the package define bin and there are more than 1 {x}Version.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, but even when there is a bin field, this is optional. The nesting can be not required. Like in the dependencies in Cargo.toml. For instance:

{
  "pnpm": {
    "executionEnv": {
      "runtime": {
        "deno": "1.1.0",
        "node": {
          "version": "20.16.0",
          "cli": true
        }
    }
  }
}


## Motivation

Running multiple versions of Node.js on the same computer isn't easy. Also, there is currently no way for a package to tell the package manager that it needs to be executed with a specific version of Node.js. Node.js versions should be locked the same way as other dependencies of projects are locked for reproducibility.
Copy link

Choose a reason for hiding this comment

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

There is already a way: engines

https://docs.npmjs.com/cli/v10/configuring-npm/package-json#engines

Perhaps this RFC could explain why engines is not sufficient and why we need a new feature.

Copy link
Member Author

Choose a reason for hiding this comment

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

As you can see in the alternatives section:

Instead of introducing a new field, we could use the engines field for detecting what Node.js version should be used for running the bin file or building the package. However, the engines field is already used by other package managers and it is usually just sets a range with the lowest supported Node.js version. If we will use it for specifying exact versions, installations of the package with other package managers will fail, when engine-strict is set to true.

@zkochan
Copy link
Member Author

zkochan commented Aug 28, 2024

So, in the openjs-foundation/package-metadata-interoperability-collab-space#15 RFC it was suggested that we leverage the already existing "engines" field. Which I don't know if it is a good idea but I think we can do it for globally installed CLI tools. I would put an exact version to the pnpm CLI package's engines field for Node.js and it would make pnpm more stable, when installed with pnpm.

The good thing about using the engines field is that it is already used by the ecosystem, so all the existing CLI tools would benefit from it. But all the tools use ranges currently, so the "stability" bonus wouldn't be so effective. Nevertheless, it could be a good starting point to start with "engines".

@antitoxic
Copy link

antitoxic commented Aug 28, 2024

👋 Hey there, first let me say that I love this RFC and the fact that pnpm can unlock workspaces that have multiple runtimes like a package intended for node and a package intended for bun.

At the company i'm working at we are already using pnpm.executionEnv.nodeVersion and here's the feedback from our end:

Some background of my thinking

jsRuntime sounds good to me

Actually I think the naming of this can be a bit misleading. Yes it is a JS runtime but browsers are also JS runtimes. And many npm packages are design to ultimately run in the browser. However 🙂 their dev lifecycle will happen through CLI tools for things like build, test, lint, scripts any anything in the scripts section of their package.json.

So in this regard the issue @zkochan posted openjs-foundation/package-metadata-interoperability-collab-space#15 suggest an interesting name: devEngines. There are packages that are NOT intended for the browser and will be used in nodejs/bun/deno/cloudflare, etc, BUT there already is a field in package.json that defines the required/intended environment for runtime and that is engines. This of course right now only relates to nodejs version, not other runtimes.

Bringing in a thread from a recent pnpm PR

Continuing the same logic I want to also reply to a comment by @zkochan of a message I had in another PR pnpm/pnpm#8277, regarding lifecycles with pnpm.executionEnv.nodeVersion and specifically:

... 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?

For me the answer is obvious: all dev-related processes of a package like build, postinstall, its scripts in package.json should all be executed using the defined pnpm.executionEnv.nodeVersion by that package.

The runtime for when the package is used should be controlled by whoever is using it, and the requirements can be defined (currently) in the engines property.

The case of cli commands exposed by a package

For the scripts defined in the bin property of package.json our company currently solved this with a bit of a workaround and we can pick which runtime we use on case-by-case basis where we have a sensible (for us) default.

Example setup
{
  "name": "packageAUsingNode14",
  "engines": {
    "node": "> 14"
  },
  "bin": {
    "package-a-bin-cmd": "./myCliCmd.js"
  },
  "scripts": {
    "package-a-bin-com:from-external": "cd $INIT_CWD && ./myCliCmd.js"
  },
  "pnpm": {
    "executionEnv": {
      "nodeVersion": "14.21.3"
    }
  }
}
{
  "name": "appBUsingNode18",
  "pnpm": {
    "executionEnv": {
      "nodeVersion": "18.2.0"
    }
  },
  "dependencies": {
    "packageAUsingNode14": "workspace:^"
  },
  "scripts": {
    "runPackageAUsingNode18": "package-a-bin-cmd",
    "runPackageAUsingNode14": "pnpm --filter='packageAUsingNode14' run package-a-bin-com:from-external"
  }
}

So by default bin CLI scripts like package-a-bin-cmd are running using the runtime of whatever installed packageAUsingNode14 but if you really want to use the node version that the packageAUsingNode14 was developed in while running the CMD you can do that by pnpm --filter='packageAUsingNode14' run ... and have a script in packageAUsingNode14 that simply executes the script.

You can see runPackageAUsingNode18 and runPackageAUsingNode14 in appBUsingNode18. Both running the same command from the same packageAUsingNode14 but with different node versions.

Suggestion for this RFC

In my mind i can see something like:

{
  "name": "my-package-name",
  "engines": {
    "node": ">18",
    "bun": ">0.5"
  },
  "scripts": {
    "postinstall": "... using bun 1.2.0...",
    "preinstall": "... using bun 1.2.0...",
    "any other script": "... using bun 1.2.0...",
    "preany other script": "... using bun 1.2.0...",
    "postany other script": "... using bun 1.2.0..."
  },
  "bin": {
    "my-cli-cmd": "... using whatever devEngine the consumer package has by default...",
  },
  "pnpm": {
    "devEngines": {
      "bun": "1.2.0"
    }
  }
}

So this allows the package to live in its own isolated world when developed or when its scripts are run but also allow to define its consumer-restrictions using the engines field. Both doesn't need to be the same. Maybe there could be better than our workaround mechanism for running bin/cli scripts but I can't think of it right now. Maybe cliEngines at the same level as devEngines within pnpm?

@antitoxic
Copy link

PS: Our implementation is a bit different from the examples I've given above regarding CLI commands since we have a similar case but not exactly that one: we execute eslint in different packages using a different node version.

So instead "cd $INIT_CWD && ./myCliCmd.js" we have "cd $INIT_CWD && eslint" where eslint is already in $PATH and using node14 when executed from a package that has node14 as its executionEnv.nodeVersion.

@zkochan
Copy link
Member Author

zkochan commented Aug 28, 2024

As you can see in this post from past core npm maintainer, he suggests to not allow build scripts to pick the runtime.

@antitoxic
Copy link

@zkochan I'm not sure I fully understand the comment. It's referring to Package Distributions but that's not a thing yet and for sure it will never be a thing for older runtime versions than whenever it's released, right?

@zkochan
Copy link
Member Author

zkochan commented Aug 28, 2024

he says they consider postinstall scripts a 'legacy feature' and don't want us to make it easier using it.

@antitoxic
Copy link

@zkochan ok but what does that mean for all existing packages and their older versions that will not just disappear? I thought the whole idea of making isolated packages have their own processing environment is to allow gradual isolated changes and not force upgrades of everything to the new standard of having dependencies that don't use post/pre install scripts

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.

4 participants