nodejs_binary prefers ES module exports over CommonJS

[leops]

🐞 bug report

Affected Rule

The issue is caused by the rule: nodejs_binary

Is this a regression?

No

Description

The nodejs_binary rule patches the require function of Node.js in a way that tries to load resolve extension-less files as ES module files (.mjs extension) before CommonJS modules (.js extension), even though ES Modules cannot be loaded using require

🔬 Minimal Reproduction

I originally hit this problem on code using the graphql library, which exposes both CommonJS and ESM entrypoints so the minimal reproduction case starting from npx @bazel/create is simply a yarn add graphql and adding a nodejs_binary rule that loads a file doing require('graphql')

Repository: https://github.com/leops/bazel_nodejs_esm_repro

🔥 Exception or Error

[bin_require_patch.js] patching require for //:bin
  cwd: c:\users\leops\_bazel~1\zag5q67q\execroot\repro\bazel-~1\x64_wi~1\bin\binbat~2.run
  RUNFILES: C:\Users\leops\_bazel_leops\zag5q67q\execroot\repro\bazel-out\x64_windows-fastbuild\bin\bin.bat.runfiles\MANIFEST
  TARGET: //:bin
  BIN_DIR: bazel-out/x64_windows-fastbuild/bin
  GEN_DIR: bazel-out/x64_windows-fastbuild/bin
  INSTALL_SOURCE_MAP_SUPPORT: true
  MODULE_ROOTS: []
  NODE_MODULES_ROOT: npm/node_modules
  USER_WORKSPACE_NAME: repro

[bin_require_patch.js] using manifest C:/Users/leops/_bazel_leops/zag5q67q/execroot/repro/bazel-out/x64_windows-fastbuild/bin/bin.bat.runfiles/MANIFEST
[bin_require_patch.js] using outputBase C:/Users/leops/_bazel_leops/zag5q67q
[bin_require_patch.js] using binRoot C:/Users/leops/_bazel_leops/zag5q67q/execroot/repro/bazel-out/x64_windows-fastbuild/bin/
[bin_require_patch.js] using genRoot C:/Users/leops/_bazel_leops/zag5q67q/execroot/repro/bazel-out/x64_windows-fastbuild/bin/
[bin_require_patch.js] using localWorkspacePath C:/users/leops/_bazel_leops/zag5q67q/execroot/repro/bazel-out/x64_windows-fastbuild/bin/
[bin_require_patch.js] try to resolve in runfiles manifest c:/users/leops/_bazel~1/zag5q67q/execroot/repro/bazel-~1/x64_wi~1/bin/build_bazel_rules_nodejs/third_party/github.com/source-map-support
[bin_require_patch.js] try to resolve in runfiles manifest npm/node_modules/c:/users/leops/_bazel~1/zag5q67q/execroot/repro/bazel-~1/x64_wi~1/bin/build_bazel_rules_nodejs/third_party/github.com/source-map-support
[bin_require_patch.js] WARNING: source-map-support module not installed.
      Stack traces from languages like TypeScript will point to generated .js files.
      Set install_source_map_support = False in //:bin to turn off this warning.
[bin_require_patch.js] try to resolve in runfiles manifest repro/index.js
[bin_require_patch.js] resolved manifest file C:/users/leops/documents/projects/repro/index.js
[bin_require_patch.js] resolved repro/index.js within runfiles to C:\users\leops\documents\projects\repro\index.js from undefined
[bin_require_patch.js] try to resolve in runfiles manifest graphql
[bin_require_patch.js] try to resolve in runfiles manifest npm/node_modules/graphql
[bin_require_patch.js] resolved via manifest directory C:/users/leops/_bazel_leops/zag5q67q/external/npm/node_modules/graphql/index.mjs
[bin_require_patch.js] resolved graphql within node_modules (npm/node_modules) to C:\users\leops\_bazel_leops\zag5q67q\external\npm\node_modules\graphql\index.mjs from C:\users\leops\documents\projects\repro\index.js
Error [ERR_REQUIRE_ESM]: Must use import to load ES Module: C:\users\leops\_bazel_leops\zag5q67q\external\npm\node_modules\graphql\index.mjs
    at Object.Module._extensions..mjs (internal/modules/cjs/loader.js:1007:9)
    at Module.load (internal/modules/cjs/loader.js:812:32)
    at Function.Module._load (internal/modules/cjs/loader.js:724:14)
    at Module.require (internal/modules/cjs/loader.js:849:19)
    at require (internal/modules/cjs/helpers.js:74:18)
    at Object. (C:\users\leops\documents\projects\repro\index.js:1:1)
    at Module._compile (internal/modules/cjs/loader.js:956:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:973:10)
    at Module.load (internal/modules/cjs/loader.js:812:32)
    at Function.Module._load (internal/modules/cjs/loader.js:724:14)

🌍 Your Environment

Operating System:

Windows 10
node.js v12.13.1

Output of bazel version:

Bazelisk version: v1.5.0
Build label: 3.0.0
Build target: bazel-out/x64_windows-opt/bin/src/main/java/com/google/devtools/build/lib/bazel/BazelServer_deploy.jar
Build time: Mon Apr 6 12:56:01 2020 (1586177761)
Build timestamp: 1586177761
Build timestamp as int: 1586177761

Rules_nodejs version:

1.7.0

[github-actions]

This issue has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs in two weeks. Collaborators can add a "cleanup" or "need: discussion" label to keep it open indefinitely. Thanks for your contributions to rules_nodejs!

[leops]

The error still happens on rules_nodejs 2.1.0 with the following bazel version:

Bazelisk version: v1.6.1
Build label: 3.0.0
Build target: bazel-out/x64_windows-opt/bin/src/main/java/com/google/devtools/build/lib/bazel/BazelServer_deploy.jar
Build time: Mon Apr 6 12:56:01 2020 (1586177761)
Build timestamp: 1586177761
Build timestamp as int: 1586177761

I've updated the reproduction repository accordingly.

[quasicomputational]

This is ultimately due to #315, I think - the require patch is apparently not needed but nodejs_binary hasn't been told the good news yet. (I got here by being bitten by something similar.)

[alexeagle]

@quasicomputational I agree #315 is the root cause. #2125 will make the patching of the require function opt-in. Meantime (for 2.x) the workaround is to add templated_args = ["--nobazel_patch_module_resolver"] to the nodejs_binary

[github-actions]

This issue has been automatically marked as stale because it has not had any activity for 90 days. It will be closed if no further activity occurs in two weeks. Collaborators can add a "cleanup" or "need: discussion" label to keep it open indefinitely. Thanks for your contributions to rules_nodejs!

[github-actions]

This issue was automatically closed because it went two weeks without a reply since it was labeled "Can Close?"