refactor(builtin): determine correct root for linking packages

This should work better than trial-and-error inside the process.

internal/js_library/js_library.bzl

@@ -60,6 +60,7 @@ js_library = rule(
     attrs = {
         "srcs": attr.label_list(allow_files = [".js"]),
         "amd_names": attr.string_dict(doc = _AMD_NAMES_DOC),
+        "module_from_src": attr.bool(),
         # Used to determine module mappings
         "module_name": attr.string(),
         "module_root": attr.string(),

internal/linker/link_node_modules.bzl

@@ -34,7 +34,8 @@ def register_node_modules_linker(ctx, args, inputs):
     mappings = {
         # We always map the workspace to itself to support absolute require like
         # import from 'my_wksp/path/to/file'
-        ctx.workspace_name: ctx.workspace_name,
+        # and it's always a bin dependency, TODO: let user control via package.json
+        ctx.workspace_name: ["bin", ctx.workspace_name],
     }
     node_modules_root = ""
 
@@ -69,7 +70,7 @@ def register_node_modules_linker(ctx, args, inputs):
     args.add("--bazel_node_modules_manifest=%s" % modules_manifest.path)
     inputs.append(modules_manifest)
 
-def get_module_mappings(label, attrs, vars, srcs = [], workspace_name = None):
+def get_module_mappings(label, attrs, vars, rule_kind, srcs = [], workspace_name = None):
     """Returns the module_mappings from the given attrs.
 
     Collects a {module_name - module_root} hash from all transitive dependencies,
@@ -95,7 +96,18 @@ def get_module_mappings(label, attrs, vars, srcs = [], workspace_name = None):
                     fail(("duplicate module mapping at %s: %s maps to both %s and %s" %
                           (label, k, mappings[k], v)), "deps")
                 _debug(vars, "target %s propagating module mapping %s: %s" % (dep, k, v))
-                mappings[k] = v
+
+                # A package which was reachable transitively via a *_binary
+                # rule is assumed to be in the runfiles of that binary,
+                # so we switch the linker target root.
+                # In theory we ought to be able to tell that the files really are
+                # propagated through the runfiles output of the target we are visiting
+                # but it seems that this info is only available in Bazel Java internals.
+                # TODO: revisit whether this is the correct condition after downstream testing
+                if rule_kind.endswith("_binary"):
+                    mappings[k] = ["runfiles", v[1]]
+                else:
+                    mappings[k] = v
 
     if not getattr(attrs, "module_name", None) and not getattr(attrs, "module_root", None):
         # No mappings contributed here, short-circuit with the transitive ones we collected
@@ -114,7 +126,15 @@ def get_module_mappings(label, attrs, vars, srcs = [], workspace_name = None):
         fail(("duplicate module mapping at %s: %s maps to both %s and %s" %
               (label, mn, mappings[mn], mr)), "deps")
     _debug(vars, "target %s adding module mapping %s: %s" % (label, mn, mr))
-    mappings[mn] = mr
+
+    # since our module mapping is currently based on attribute names,
+    # allow a special one to instruct the linker that the package has no output
+    # directory and is therefore meant to be used as sources.
+    # TODO: This belongs in a different mechanism like a package.json field.
+    if getattr(attrs, "module_from_src", False):
+        mappings[mn] = ["src", mr]
+    else:
+        mappings[mn] = ["bin", mr]
     return mappings
 
 # When building a mapping for use at runtime, we need paths to be relative to
@@ -137,6 +157,7 @@ def _module_mappings_aspect_impl(target, ctx):
             target.label,
             ctx.rule.attr,
             ctx.var,
+            ctx.rule.kind,
             workspace_name = workspace_name,
         ),
     })

internal/linker/link_node_modules.ts

@@ -216,6 +216,14 @@ async function exists(p: string) {
   }
 }
 
+// See link_node_modules.bzl where these three strings
+// are used to indicate which root the linker should target
+// for each package:
+// bin: bazel-bin/path/to/package
+// src: workspace/path/to/package
+// runfiles: look in the runfiles dir/manifest
+type LinkerRoot = 'bin'|'src'|'runfiles';
+
 export async function main(args: string[], runfiles: Runfiles) {
   if (!args || args.length < 1)
     throw new Error('link_node_modules.js requires one argument: modulesManifest path');
@@ -260,30 +268,47 @@ export async function main(args: string[], runfiles: Runfiles) {
   const links = [];
 
   const linkModule =
-      async (name: string, modulePath: string) => {
-    let target = runfiles.resolve(modulePath);
-
-    // It sucks that we have to do a FS call here.
-    // TODO: could we know which packages are statically linked??
-    if (!target || !await exists(target)) {
-      // Try the bin dir
-      target = path.join(workspaceRelative, bin, toWorkspaceDir(modulePath));
-      if (!await exists(target)) {
-        // Try the execroot
+      async (name: string, root: LinkerRoot, modulePath: string) => {
+    let target: string = '<package linking failed>';
+    switch (root) {
+      case 'bin':
+        // FIXME(#1196)
+        target = path.join(workspaceRelative, bin, toWorkspaceDir(modulePath));
+        // Spend an extra FS lookup to give better error in this case
+        if (!await exists(target)) {
+          // TODO: there should be some docs explaining how users are
+          // expected to declare ahead of time where the package is loaded,
+          // how that relates to npm link scenarios,
+          // and where the configuration can go.
+          return Promise.reject(`ERROR: no output directory found for package ${modulePath}
+        Did you mean to declare this as a from-source package?
+        See https://github.com/bazelbuild/rules_nodejs/pull/1197
+        until this feature is properly documented.`);
+        }
+        break;
+      case 'src':
         target = path.join(workspaceRelative, toWorkspaceDir(modulePath));
-      }
+        break;
+      case 'runfiles':
+        target = runfiles.resolve(modulePath) || '<runfiles resolution failed>';
+        break;
     }
 
     await symlink(target, name);
   }
 
   for (const m of Object.keys(modules)) {
-    links.push(linkModule(m, modules[m]));
+    const [kind, modulePath] = modules[m];
+    links.push(linkModule(m, kind, modulePath));
   }
 
-  await Promise.all(links);
+  let code = 0;
+  await Promise.all(links).catch(e => {
+    console.error(e);
+    code = 1;
+  });
 
-  return 0;
+  return code;
 }
 
 export const runfiles = new Runfiles(process.env);

internal/linker/test/integration/BUILD.bazel

@@ -18,7 +18,6 @@ sh_binary(
     name = "run_program",
     srcs = ["run_program_with_node.sh"],
     data = [
-        "absolute_import/index.js",
         ":program.js",
         "//internal/linker:index.js",
         "//internal/linker/test/integration/static_linked_pkg",
@@ -33,6 +32,7 @@ linked(
     out = "actual",
     program = ":run_program",
     deps = [
+        "//%s/absolute_import:index.js" % package_name(),
         ":run_program",
         "//internal/linker/test/integration/dynamic_linked_pkg",
         "@npm//semver",

internal/linker/test/integration/absolute_import/BUILD.bazel

@@ -0,0 +1,8 @@
+package(default_visibility = ["//internal/linker/test:__subpackages__"])
+
+genrule(
+    name = "copy_to_bin",
+    srcs = ["index.js_"],
+    outs = ["index.js"],
+    cmd = "cp $< $@",
+)

internal/linker/test/integration/dynamic_linked_pkg/BUILD.bazel

@@ -5,5 +5,6 @@ package(default_visibility = ["//internal/linker/test:__subpackages__"])
 js_library(
     name = "dynamic_linked_pkg",
     srcs = ["index.js"],
+    module_from_src = True,
     module_name = "dynamic_linked",
 )

internal/linker/test/integration/rule.bzl

@@ -18,5 +18,8 @@ def _linked(ctx):
 linked = rule(_linked, attrs = {
     "out": attr.output(),
     "program": attr.label(executable = True, cfg = "host", mandatory = True),
-    "deps": attr.label_list(aspects = [module_mappings_aspect]),
+    "deps": attr.label_list(
+        allow_files = True,
+        aspects = [module_mappings_aspect],
+    ),
 })

internal/linker/test/link_node_modules.spec.ts

@@ -69,9 +69,9 @@ describe('link_node_modules', () => {
     writeManifest({
       'bin': BIN_DIR,
       'modules': {
-        'a': `${workspace}/path/to/lib_a`,
-        'b': 'other_wksp/path/to/lib_b',
-        'c': `${workspace}/path/to/lib_c`,
+        'a': ['src', `${workspace}/path/to/lib_a`],
+        'b': ['src', 'other_wksp/path/to/lib_b'],
+        'c': ['bin', `${workspace}/path/to/lib_c`],
       },
       'workspace': workspace,
     });

packages/rollup/test/integration_e2e_rollup/fum/BUILD.bazel

@@ -5,6 +5,7 @@ package(default_visibility = ["//packages/rollup:__subpackages__"])
 js_library(
     name = "fumlib",
     srcs = ["index.js"],
+    module_from_src = True,
     module_name = "fumlib",
     # Don't allow deep imports under here,
     # and give it the AMD name "fumlib", not "fumlib/index"