[Bug]: Transitive npm deps not propagated correctly to container image

[mvukov]

What happened?

Looks like ts_project -> js_binary -> docker image misses routing transitive npm deps. Reproduced a bug in https://github.com/oqton/rules_ts/tree/feature/rules_docker_bug.

The issue looks very similar to https://github.com/aspect-build/rules_jest/issues/72 to me.

Version

Development (host) and target OS/architectures: Ubuntu 20.04

Output of bazel --version: 5.2.0

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

• rules_ts: 1.0.2

Language(s) and/or frameworks involved:

How to reproduce

$ bazel run //app:image
$ docker run -it --rm bazel/app:image
node:internal/modules/cjs/loader:936
  throw err;
  ^

Error: Cannot find module 'ts-md5'
Require stack:
- /app/app/app.sh.runfiles/rules_jest_ts_bug/node_modules/.aspect_rules_js/@[email protected]/node_modules/@oqton/lib1/index.js
- /app/app/app.sh.runfiles/rules_jest_ts_bug/app/main.js
    at Function.Module._resolveFilename (node:internal/modules/cjs/loader:933:15)
    at Function.Module._load (node:internal/modules/cjs/loader:778:27)
    at Module.require (node:internal/modules/cjs/loader:1005:19)
    at require (node:internal/modules/cjs/helpers:102:18)
    at Object.<anonymous> (/app/app/app.sh.runfiles/rules_jest_ts_bug/node_modules/.aspect_rules_js/@[email protected]/node_modules/@oqton/lib1/index.js:4:18)
    at Module._compile (node:internal/modules/cjs/loader:1105:14)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1159:10)
    at Module.load (node:internal/modules/cjs/loader:981:32)
    at Function.Module._load (node:internal/modules/cjs/loader:822:12)
    at Module.require (node:internal/modules/cjs/loader:1005:19) {
  code: 'MODULE_NOT_FOUND',
  requireStack: [
    '/app/app/app.sh.runfiles/rules_jest_ts_bug/node_modules/.aspect_rules_js/@[email protected]/node_modules/@oqton/lib1/index.js',
    '/app/app/app.sh.runfiles/rules_jest_ts_bug/app/main.js'
  ]
}

Any other information?

No response

Fund our work

• [ ] Sponsor our open source work by donating a bug bounty

[thesayyn]

Duplicate https://github.com/aspect-build/rules_js/issues/677

[flolu]

@mvukov I've seen that you have created a PR for a fix. Is there any workaround until this PR is merged?

[mvukov]

@flolu Sorry, just saw this msg. You can use oqton remote, or, just download that commit, export as a patch, and patch the http_archive (if you want to use the official remote).

[flolu]

@mvukov thanks! I did not know that it is possible to apply patches to an http_archive

[gregmagolan]

The root cause here is that npm dependencies in the deps list of ts_project targets are not automatically added as transitive npm deps of downstream npm_package targets when they are linked.

Depending on a ts_project directly works as expected with transitive npm deps being propagated to runfiles. When going through a linked npm_package, however, the linker doesn't automatically link all npm packages in the transitive closure of an npm_package's srcs as transitive npm deps.

Presently you have to specify the transitive npm dependencies to link when linking an npm_package in the data attribute of build rules such as ts_project or the data attribute of the npm_package that is being linked.

For example, the principled fix with the current behaviour in https://github.com/flolu/rules-ts-transitive-deps (repro of https://github.com/aspect-build/rules_js/issues/677) is:

$ git diff
diff --git a/lib1/BUILD.bazel b/lib1/BUILD.bazel
index d3bebbe..f65a445 100644
--- a/lib1/BUILD.bazel
+++ b/lib1/BUILD.bazel
@@ -19,6 +19,7 @@ ts_project(
 npm_package(
     name = "lib1",
     srcs = [":lib1_project"],
+    data = ["//:node_modules/@org/lib2"],
     package = "@org/lib1",
     visibility = ["//visibility:public"],
 )
diff --git a/lib2/BUILD.bazel b/lib2/BUILD.bazel
index f6c6199..a635ea0 100644
--- a/lib2/BUILD.bazel
+++ b/lib2/BUILD.bazel
@@ -20,5 +20,6 @@ npm_package(
     name = "lib2",
     srcs = [":lib2_project"],
     package = "@org/lib2",
+    data = ["//:node_modules/@faker-js/faker"],
     visibility = ["//visibility:public"],
 )

and the principled fix for https://github.com/oqton/rules_jest_ts_bug (repro for https://github.com/aspect-build/rules_jest/issues/72) is

$ git diff
diff --git a/lib1/BUILD.bazel b/lib1/BUILD.bazel
index 1104029..ae9ade5 100644
--- a/lib1/BUILD.bazel
+++ b/lib1/BUILD.bazel
@@ -8,6 +8,9 @@ ts_project(
     deps = [
         "//:node_modules/ts-md5",
     ],
+    data = [
+        "//:node_modules/ts-md5",
+    ],
     declaration = True,
     tsconfig = {},
     extends = "//:tsconfig",

This behaviour was intentional to give users fine grained control over which deps should be linked as transitive deps of downstream linked npm_package targets. In retrospect, controlling if an npm package is a transitive linked dependency should have been an opt-out instead of an opt-in. The default behaviour should be the happy path with an option still available for users that want to control what npm deps are linked as transitive npm deps.

This discussion also came up recently with work that @jbedard has been doing. I've been considering the best API change here and I'm currently leaning towards adding a dev_deps (or perhaps dev_npm_deps) attribute to all rules_js build rules (js_library, ts_project, etc...) that would allow specifying npm deps that are not propagated as linked npm deps. With that added, deps could be changed to always propagate npm deps as transitive deps when linking which is the effect that https://github.com/aspect-build/rules_ts/pull/280 has.

NB: In the https://github.com/flolu/rules-ts-transitive-deps case, with the transpiler set on ts_project, it looks like there is a bug with data propagation when the transpiler is set so setting the dep on the npm_package is necessary in that case. I'll fix that while improving the default propagation behaviour of deps.

[mvukov]

I'd like to understand what's sub-optimal with my solution in https://github.com/aspect-build/rules_ts/pull/280. In the problematic cases that I experienced, the situation is pretty much simple: A depends on B, B depends on C. With the current ruleset, A cannot see C, while deps are properly routed via deps rules/macro attribute. Other way around, why is npm_package so special to require a special treatment, that is at least to me, non-intuitive? Note: my js/ts is fairly slim, to be fair.

In straightforward use-cases I had (such as https://github.com/oqton/rules_jest_ts_bug), having something like dev_deps doesn't look intuitive as all deps are actually used in runtime.

[gregmagolan]

The solution we end up with might end up looking similar to #280, however, the problem space needs a little thought and experimentation first. This shape of this problem, "what deps to pass forward through a downstream linked npm_package?", is not unique to ts_project. A change in behaviour here would likely mean a change in js_library in rules_js as well to keep behaviour consistent and potentially other downstream rules from rules_js that provide JsInfo (we have a number of downstream rule sets: https://github.com/aspect-build).

npm_package itself it not special (its just a packaging rule that copies files to a directory output). Depending on a linked copy of npm_package however, like you do in your repro, is quite different than depending on a ts_project directly.

When you do that, your dependency is a copy of the contents of the npm_package in the virtual store (a special symlinked layout of node_modules similar to what pnpm uses) via a symlink in the output tree such as node_modules/@oqton/lib1. third-party npm deps such as ts-md5 that were in the graph to build npm_package (resolved from node_modules/ts-md) that you want to forward to consumers of the node_modules/@oqton/lib1 also also not depended on directly. Instead their underlying virtual store directories are symlinked as transitive deps in @oqton/lib1's location in the virtual store. Passing these deps forward to the linker so is done via the JsInfo npm_package_store_deps field and the pattern that ts_project uses to collect these, which #280 modified, is shared with other rules such as js_library: https://github.com/aspect-build/rules_js/blob/01111b254e0292f57633253d54803ede955c5018/js/private/js_library.bzl#L218

The underlying mechanics of "linking" in rules_js is similar to how pnpm/yarn and npm workspaces work where first-party packages are symlinked into the "node_modules" trees and then imported by their "linked" names and resolved by standard node_modules resolution via their symlinked locations in the node_module tree. It can be thought of as local short-cut to publishing a first-party to an npm registry and then consuming it in the build via a package.json reference. pnpm/yarn & npm workspaces and rules_js hide all of the underlying mechanics. In rules_js, there is quite a bit going on under the hood when depending on linked target vs depending an a target directly.

When using workspaces outside of Bazel, a developer must explicitly specify what are the "devDependencies" of a npm package and what are the "dependencies" that are passed on to consumers of the package in the package's package.json (https://docs.npmjs.com/specifying-dependencies-and-devdependencies-in-a-package-json-file). When you pull a package down with yarn/npm/pnpm, the package managers will install only the "dependencies" specified in the npm package and not the "devDependencies" since those may only have been needed when for building & publishing/linking the package.

You can encounter a similar situation in rules_js where some deps are only needed for building the npm_package and are not needed by consumers of the linked package. @types/* deps are a common example as they are only needed for type-checking by ts_project itself and not at runtime. Not forwarding all deps in the graph of npm_package through the linker was done with that in mind since not all deps are needed by consumers. The dev_deps proposal is one way to make the default behaviour simpler but still allow users to use npm dependencies for building npm packages that don't automatically (and for some users undesirably) become transitive deps of the npm package when linked.

@alexeagle @jbedard and I are still debating what the best way to express this in the API is. We'll come up with something in the near future since the solution will also intersect with BUILD file generation for rules_js that @jbedard is currently working on.

[mvukov]

Thanks, @gregmagolan ! Highly appreciated. OK, splitting devs/dev_deps makes sense now and should be more in-line how non-Bazel js tooling works. (IIUC, that should also reduce the amount of .d.ts files in a docker image of a nodejs app).

[gregmagolan]

Yes, dev_deps wouldn't propagate through a linked npm_package so if is used within a js_image_layer then they won't be included. It may also be possible to configure js_image_layer & js_binary such that dev_deps of direct ts_project and js_library deps don't end up in the layer either. That rule is currently being promoted to the public API: https://github.com/aspect-build/rules_js/pull/771. FYI @thesayyn

[gregmagolan]

@jbedard and I came up with an even simpler plan that uses the classification of "dev" dependencies that is already encoded in the pnpm lock file: https://github.com/aspect-build/rules_js/pull/860. Once that lands, a corresponding change is needed in rules_ts that will finally fix this issue.

[gregmagolan]

Fix landed finally and is available at HEAD. Thanks for your patience on this one. It needed some thought to get right to avoid more churn.

I've got a few more things I'd like to get in before cutting a 1.3.0 release. For now you can get the fix using the HEAD commit hash.