Issue with nodejs_binary typescript entry_point defined as label (0.31.1 release)

[siberex]

🐞 bug report

Affected Rule

The issue is caused by the rule: nodejs_binary

Is this a regression?

Yes, the previous version in which this bug was not present was: 0.30.2

Description

After switching from string-defined entry_point (0.30.2):

entry_point = "workspace_name/src/main.js",

to label (0.31.1):

entry_point = ":main.ts",

bazel run produces error: No file workspace_name/src/main.js found in module root workspace_name/src/src/main.js.

For some reason it is looking for a js file under /src/src/.

🔬 Minimal Reproduction

Use this repo to reproduce: https://github.com/siberex/bazel_issue_rules_nodejs_entry_point

git clone [email protected]:siberex/bazel_issue_rules_nodejs_entry_point.git /tmp/entry_point_label
cd /tmp/entry_point_label
bazel run //src:server

🔥 Exception or Error

INFO: Build completed successfully, 6 total actions
Error: No file entry_point_label/src/main.js found in module root entry_point_label/src/src/main.js
    at Function.module.constructor._resolveFilename (/private/var/tmp/_bazel_sib/5848ea03bd44a0920c5f5eda67e7a2b2/execroot/entry_point_label/bazel-out/darwin-fastbuild/bin/src/server_bin_loader.js:362:13)
    at Function.Module._load (internal/modules/cjs/loader.js:506:25)
    at Object. (/private/var/tmp/_bazel_sib/5848ea03bd44a0920c5f5eda67e7a2b2/execroot/entry_point_label/bazel-out/darwin-fastbuild/bin/src/server_bin_loader.js:501:24)
    at Module._compile (internal/modules/cjs/loader.js:688:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:699:10)
    at Module.load (internal/modules/cjs/loader.js:598:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:537:12)
    at Function.Module._load (internal/modules/cjs/loader.js:529:3)
    at Function.Module.runMain (internal/modules/cjs/loader.js:741:12)
    at startup (internal/bootstrap/node.js:285:19)

🌍 Your Environment

  
Mac OS 10.14.5
  

Output of bazel version:

  
Build label: 0.26.1
Build target: bazel-out/darwin-opt/bin/src/main/java/com/google/devtools/build/lib/bazel/BazelServer_deploy.jar
Build time: Thu Jun 6 11:08:11 2019 (1559819291)
Build timestamp: 1559819291
Build timestamp as int: 1559819291
  

Rules version (SHA):

  
e04a82a72146bfbca2d0575947daa60fda1878c8d3a3afe868a8ec39a6b968bb
  

Anything else relevant?

Workaround for this is to add src/src/main.ts:

import '../main';

[gregmagolan]

Thanks for the report & repro. Thats a nice catch. The underlying cause is that the path to main.js is entry_point_label/src/main.js in rufiles but there is a module_name mapping of entry_point_label => entry_point_label/src because of module_name in

ts_library(
    name = "app",
    srcs = glob(
        include = ["**/*.ts"],
        exclude = ["**/*spec.ts"],
    ),
    module_name = "entry_point_label",
)

The node resolver doesn't handle this case properly and throws thinking that the fully qualified entry_point path of entry_point_label/src/main.js is a actually a path within the entry_point_label module which expands to entry_point_label/src/src/main.js

[gregmagolan]

I think two fixes are needed here. First, the node resolver should not look at module roots when resolving a main file:

  if (!isMain) {
    const moduleRoot = resolveToModuleRoot(request);
    if (moduleRoot) {
      const moduleRootInRunfiles = resolveRunfiles(undefined, moduleRoot);
      const filename = module.constructor._findPath(moduleRootInRunfiles, []);
      if (!filename) {
        throw new Error(`No file ${request} found in module root ${moduleRoot}`);
      }
      return filename;
    }
  }

second, we can move the runfiles resolve

// If the import is not a built-in module, an absolute, relative import or a
  // dependency of an npm package, attempt to resolve against the runfiles location
  try {
    const resolved = originalResolveFilename(resolveRunfiles(parentFilename, request), parent, isMain, options);
    if (DEBUG)
      console.error(
          `node_loader: resolved ${request} within runfiles to ${resolved} from ${parentFilename}`
      );
    return resolved;
  } catch (e) {
    failedResolutions.push(`runfiles - ${e.toString()}`);
  }

to before module root resolve so that you can have fully qualified require require('module_name/path/to/src/foo.js)and the mapping ofmodule_name=>module_name/path/to/src` won’t break things