[Bug]: dev = True support missing from pnpm version 9 lockfiles

[sin-ack]

What happened?

I added the following to my MODULE.bazel:

npm = use_extension("@aspect_rules_js//npm:extensions.bzl", "npm")
npm.npm_translate_lock(
    name = "npm_dev",
    npmrc = "//:.npmrc",
    pnpm_lock = "//:pnpm-lock.yaml",
    dev = True,
)
use_repo(npm, "npm_dev")

The intention here is to use this together with ts_project because right now it's including dev dependencies in its output (which might actually be related to this issue...).

However, when I add dev = True, no dependencies actually get included in the resulting linked packages, and I get errors related to missing type declarations.

Version

Development (host) and target OS/architectures: Host+target: Gentoo Linux

Output of bazel --version: 7.4.0

Version of the Aspect rules, or other relevant rules from your
WORKSPACE or MODULE.bazel file: 2.1.0

Language(s) and/or frameworks involved: JavaScript

How to reproduce

1. Create a new directory
2. pnpm init
3. pnpm i -D left-pad (any package)
4. echo 'require("left-pad"); require("fs").writeFileSync("dummy.txt", "");' > index.js
5. Add the following to MODULE.bazel:

module(
    name = "repro",
    version = "0.0.0",
)

bazel_dep(name = "aspect_rules_js", version = "2.1.0")

npm = use_extension("@aspect_rules_js//npm:extensions.bzl", "npm")
npm.npm_translate_lock(
    name = "npm_dev",
    pnpm_lock = "//:pnpm-lock.yaml",
    dev = True,
)
use_repo(npm, "npm_dev")

6. Add the following to BUILD.bazel:

load("@npm_dev//:defs.bzl", npm_dev_link = "npm_link_all_packages")
load("@aspect_rules_js//js:defs.bzl", "js_binary", "js_run_binary")

npm_dev_link(name = "node_modules_dev")

js_binary(
    name = "repro_bin",
    data = [":node_modules_dev"],
    entry_point = "index.js",
)

# Required to get a sandboxed environment (i.e. avoid local node_modules)
js_run_binary(
    name = "repro",
    tool = ":repro_bin",
    outs = ["dummy.txt"],
)

7. Add "pnpm": {"onlyBuiltDependencies": []} to package.json
8. Run bazel build repro. Get the following error:

ERROR: /home/.../tmp/repro/BUILD.bazel:12:14: JsRunBinary result.txt failed: (Exit 1): repro_bin failed: error executing JsRunBinary command (from target //:repro) bazel-out/k8-opt-exec-ST-d57f47055a04/bin/repro_bin_/repro_bin

Use --sandbox_debug to see verbose messages from the sandbox and retain the sandbox build root for debugging
node:internal/modules/cjs/loader:1143
  throw err;
  ^

Error: Cannot find module 'left-pad'
Require stack:
- /home/.../.cache/bazel/_bazel_.../4c58cc4ead2e23f682ce8e1d4c0dcef1/sandbox/linux-sandbox/614/execroot/_main/bazel-out/k8-fastbuild/bin/index.js
...

You can remove dev = True from the translate_lock call to have it working.

Any other information?

The offending line seems to be:

rules_js/npm/private/pnpm.bzl

Line 495 in a829760

dev = None, # TODO(pnpm9): must inspect importers.*.devDependencies?

[sin-ack]

I tried to solve this locally, but I'm kind of confused about how this should interact with importers. My understanding is that a package can be a development dependency in one workspace and a production dependency on another. How do workspaces interact with rules_js? Do we care about maintaining separate identities for dev and non-dev packages?

Also, it seems dev packages are filtered out twice, once during the transitive closure generation and another in helpers.get_npm_imports. The latter IMO is invalid because it's possible that a package marked as dev in our package.json is depended on by a production package or vice-versa (and in fact I did hit this while testing, where production packages depend on filtered out dev packages). We already culled the roots based on the user's request during the closure generation, we shouldn't cull individual packages from the graph.

[jbedard]

My understanding is that a package can be a development dependency in one workspace and a production dependency on another.

I think that's why pnpm v9 removed the flag completely, and why npm_translate_lock(dev, prod) as well as the transitive closure stuff is buggy atm.

Also, it seems dev packages are filtered out twice, once during the transitive closure generation and another in helpers.get_npm_imports.

I think this is still correct, although buggy atm (which I believe #2051 fixes).

The first filter during the generation simply avoids generating unused code.
The second filter is the one that does the actual dev vs prod logic of not passing dev dependencies along as transitive deps.

[sin-ack]

The second filter is the one that does the actual dev vs prod logic of not including dev dependencies in transitive deps.

But transitive dependencies should contain "dev dependencies". Whether a package is a dev dependency or not should only have bearing on the roots of the package graph, not the full graph. pnpm should already exclude transitive dependencies' own dev dependencies by simply not installing them. Am I misunderstanding something here?

[jbedard]

But transitive dependencies should contain "dev dependencies".

I think that is true up until a package gets published, or linked into a node_modules tree.

But your repro steps do seem to show a bug... when just using js[_run]_binary and js_library all dependencies (dev or not) should be propagated throughout those.

Does that sound right?

[sin-ack]

I think that is true up until a package gets published, or linked into a node_modules tree.

Not quite. See the graph here: #1879 (comment)

One package's devDependency can be another package's dependency. This is why the second pruning step is incorrect.

[jbedard]

Not quite. See the graph here: #1879 (comment)

I mean what it should do. I think you are correct that it's broken in the scenario you're pointing out in that graph (point 2 below).

I think there are 2 related issues here, both caused by rule_js misusing the lockfile dev flag.

1. dev dependencies should not be passed through linked packages, just like they are not passed through publishing of packages. Today many dev dependencies are passed through because we are misusing the lockfile dev flag.

2. npm_translate_lock(dev,prod) should filter which packages are translated based on how they are imported per package, where today this is done with the lockfile dev flag which is only true if it is a dev dependencies for all importers.