aspect-build / rules_js

High-performance Bazel rules for running Node.js tools and building JavaScript projects
https://docs.aspect.build/rules/aspect_rules_js
Apache License 2.0
310 stars 107 forks source link

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

Open sin-ack opened 2 weeks ago

sin-ack commented 2 weeks ago

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")
  1. 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"],
)
  1. Add "pnpm": {"onlyBuiltDependencies": []} to package.json
  2. 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:

https://github.com/aspect-build/rules_js/blob/a8297609b6ed6515648b3c2b9b48c0eb3b481852/npm/private/pnpm.bzl#L495

sin-ack commented 2 weeks ago

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.