[FR]: npm_link_all_packages should provide shorthands for "all non-devDependencies" and "all devDependencies"

[jfirebaugh]

What is the current behavior?

npm_link_all_packages provides you with a :node_modules target that represents all dependencies in package.json.

Describe the feature

There should also be some sort of automatically generated shorthand target for ":node_modules excluding dev deps" and "only dev deps". It's common to not want to micro-manage package dependencies in both package.json and BUILD.bazel, but still have targets to which dev deps are not accessible (most non-test targets), to avoid accidental inclusion of dev libraries in production code.

See https://bazelbuild.github.io/rules_rust/crate_universe.html#all_crate_deps for one potential API approach.

[sin-ack]

This would be awesome in combination with js_image_layer. My use-case is a TypeScript project running on Node which needs to be packaged into an image. Unfortunately, ts_project will forward all of its dependencies in JsInfo including development dependencies and will cause huge amounts of bloat in the final image (because it includes stuff like the entire TypeScript compiler). It would be possible to strip the Node.js dependencies from the output JavaScript files with js_info_files, but then there's no way for me to add only the production dependencies (if I try to split npm_translate_lock into two with prod = True and dev = True it will still fail with output collisions because some packages are shared between both).

Ideally this would just work:

npm_link_all_packages()

ts_project(
    name = "srcs_transpiled",
    srcs = glob(["*.ts"]),
    deps = [":node_modules"],
)

# Exclude development dependencies here?
js_binary(
    name = "bin",
    data = [":srcs_transpiled"],
    entry_point = "index.js",
)

# Or here?
js_image_layer(
    name = "layers",
    binary = ":bin",
    root = "/app",
)

But I'm also okay with:

npm_link_all_packages()

ts_project(
    name = "srcs_transpiled",
    srcs = glob(["*.ts"]),
    deps = [":node_modules"],
)

# Strip node dependencies for re-linking
js_info_files(
    name = "srcs_stripped_deps",
    srcs = [":srcs_transpiled"],
    include_npm_sources = False,
)

# Re-link here
js_binary(
    name = "bin",
    # Auto-generated target containing only dependencies and not devDependencies
    data = [":srcs_stripped_deps", ":node_modules_nodev"],
    entry_point = "index.js",
)

js_image_layer(
    name = "layers",
    binary = ":bin",
    root = "/app",
)

[jfirebaugh]

I just noticed that the documentation for ts_project(deps) implies that rules_ts wanted to make this feature unnecessary, at least for the case of doing ts_project(..., deps = [":node_modules"]), which is all I really want it for.

NB: Linked npm package targets that are "dev" dependencies do not forward their underlying npm_package_store target(s) through npm_package_store_deps and will therefore not be propagated to the direct dependencies of downstream linked npm_package targets.

However, this is followed by:

npm packages that come in from npm_translate_lock are considered "dev" dependencies if they are have dev: true set in the pnpm lock file. This should be all packages that are only listed as "devDependencies" in all package.json files within the pnpm workspace.

The problem with this definition of "dev" dependencies is that a 3rd party dep can be in devDependencies in one 1p dep, but in dependencies in another, or be a transitive dependency of some other dependency that's not a dev dependency. Either of those will make dev: false (or actually, I think it just gets omitted in that case). So in practice, this results in many dev dependencies getting propagated to downstream linked packages.

A better definition would be whether the direct dependency appears in the dependencies versus devDependencies section of the relevant entry in pnpm-lock.yaml's importers attribute.

[sin-ack]

The problem with this definition of "dev" dependencies is that a 3rd party dep can be in devDependencies in one 1p dep, but in dependencies in another, or be a transitive dependency of some other dependency that's not a dev dependency. Either of those will make dev: false (or actually, I think it just gets omitted in that case). So in practice, this results in many dev dependencies getting propagated to downstream linked packages.

Yes, this is the exact issue I had in #2013 but unfortunately the Aspect devs seem unresponsive. dev = True should only prune root packages, IMO; in fact pnpm lockfile v9 addresses this by moving the "dev-ness" of the dependency from the dependency itself to each dependent.

[jbedard]

I recently started working on this task and noticed exactly what you're describing.

The lockfile dev flag is unset/false if any package declares it as a non-dev dependency, where I think within a :node_modules target it should be based on the package.json.

I think npm_translate_lock(dev|prod|no_optional) can use the lockfile dev to completely ignore some packages, but the :node_modules targets need to use the dependencies vs devDependencies like you're both describing. Does that sound right?

[sin-ack]

Mostly, yeah. To make the problem explicit: In certain configurations the node_modules graph can look something like this:

flowchart
  pj[package.json] -->|devDependencies| dep1
  pj -->|dependencies| dep2
  dep2 -->|dependencies| dep1

Loading

What I've observed is that dep1 is pruned by the npm_translate_lock machinery at the moment because it's part of devDependencies of the root package, when that should only be used as a graph root when dev=True or nothing instead of explicit pruning.

[jfirebaugh]

I've been working on this as well. My progress is in #2051, which adds prod and dev attributes to npm_link_targets. This seems like a very natural API extension; npm_link_targets already returns a list of linked packages, and prod and dev attributes mirror those of npm_translate_lock.

Even if the forwarding behavior of dev dependencies for ts_project is improved, I would still want to do

ts_project(
   ...,
   deps = npm_link_targets(prod = True)
)

to avoid ts_project depending on dev dependencies that aren't needed for typechecking.