some npm deps fail when building their umd bundle

[Toxicable]

🐞 bug report

Affected Rule

npm_umd_bundle

Is this a regression?

Nope

Description

🔬 Minimal Reproduction

• yarn add crc
• bazel build @npm//node_modules/crc:crc.umd.js

🔥 Exception or Error

$ bazel build @npm//node_modules/crc:crc.umd.js
Another command (pid=9654) is running.  Waiting for it to complete on the server...
INFO: Build option --legacy_external_runfiles has changed, discarding analysis cache.
INFO: Analyzed target @npm//node_modules/crc:crc.umd.js (29 packages loaded, 3222 targets configured).
INFO: Found 1 target...
ERROR: /home/f.wiles/.cache/bazel/_bazel_f.wiles/4e95cbad76bfb4ed000904165db39268/external/npm/node_modules/crc/BUILD.bazel:169:1: Generated UMD bundle for 
crc npm package [browserify] failed (Exit 1) browserify-wrapped failed: error executing command bazel-out/host/bin/external/build_bazel_rules_nodejs/internal/npm_install/browserify-wrapped soa crc external/npm/node_modules/crc/lib/index.js ... (remaining 1 argument(s) skipped)

Use --sandbox_debug to see verbose messages from the sandbox
TypeError [ERR_INVALID_ARG_TYPE]: The "path" argument must be of type string. Received type number
    at assertPath (path.js:39:11)
    at Object.relative (path.js:1173:5)
    at e.exports (/home/f.wiles/.cache/bazel/_bazel_f.wiles/4e95cbad76bfb4ed000904165db39268/external/build_bazel_rules_nodejs/third_party/github.com/browserify/browserify/index.min.js:21:188447)
    at I (/home/f.wiles/.cache/bazel/_bazel_f.wiles/4e95cbad76bfb4ed000904165db39268/external/build_bazel_rules_nodejs/third_party/github.com/browserify/browserify/index.min.js:2:17397)
    at /home/f.wiles/.cache/bazel/_bazel_f.wiles/4e95cbad76bfb4ed000904165db39268/external/build_bazel_rules_nodejs/third_party/github.com/browserify/browserify/index.min.js:2:23094
    at /home/f.wiles/.cache/bazel/_bazel_f.wiles/4e95cbad76bfb4ed000904165db39268/external/build_bazel_rules_nodejs/third_party/github.com/browserify/browserify/index.min.js:21:168888
    at /home/f.wiles/.cache/bazel/_bazel_f.wiles/4e95cbad76bfb4ed000904165db39268/external/build_bazel_rules_nodejs/third_party/github.com/browserify/browserify/index.min.js:21:168556
    at FSReqWrap.readFileAfterClose [as oncomplete] (internal/fs/read_file_context.js:53:3)
Error: Command failed: node /home/f.wiles/.cache/bazel/_bazel_f.wiles/4e95cbad76bfb4ed000904165db39268/external/build_bazel_rules_nodejs/third_party/github.com/browserify/browserify/index.min.js external/npm/node_modules/crc/lib/index.js -p [ /home/f.wiles/.cache/bazel/_bazel_f.wiles/4e95cbad76bfb4ed000904165db39268/external/build_bazel_rules_nodejs/third_party/github.com/jhermsmeier/browserify-named-amd/named-amd.js --name crc ] -s browserify_soa_crc -o bazel-out/k8-fastbuild/bin/external/npm/node_modules/crc/crc.umd.js
    at checkExecSyncError (child_process.js:600:11)
    at Object.execFileSync (child_process.js:618:13)
    at runBrowserify (/home/f.wiles/.cache/bazel/_bazel_f.wiles/4e95cbad76bfb4ed000904165db39268/external/build_bazel_rules_nodejs/internal/npm_install/browserify-wrapped.js:59:17)
    at Object. (/home/f.wiles/.cache/bazel/_bazel_f.wiles/4e95cbad76bfb4ed000904165db39268/external/build_bazel_rules_nodejs/internal/npm_install/browserify-wrapped.js:25:1)
    at Module._compile (internal/modules/cjs/loader.js:689:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:700:10)
    at Module.load (internal/modules/cjs/loader.js:599:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:538:12)
    at Function.Module._load (internal/modules/cjs/loader.js:530:3)
    at Object. (/home/f.wiles/.cache/bazel/_bazel_f.wiles/4e95cbad76bfb4ed000904165db39268/execroot/soa/bazel-out/host/bin/external/build_bazel_rules_nodejs/internal/npm_install/browserify-wrapped_bin_loader.js:489:24)

WARNING: Due to a breaking change in rules_nodejs 0.13.0, target @build_bazel_rules_nodejs//internal/npm_install:browserify-wrapped_bin
must now declare either an explicit node_modules attribute, or
list explicit deps[] or data[] fine grained dependencies on npm labels
if it has any node_modules dependencies.
See https://github.com/bazelbuild/rules_nodejs/wiki#migrating-to-rules_nodejs-013

Target @npm//node_modules/crc:crc.umd.js failed to build

🌍 Your Environment

Operating System:

  linux_64

Output of bazel version:

  Build label: 0.25.2
Build target: bazel-out/k8-opt/bin/src/main/java/com/google/devtools/build/lib/bazel/BazelServer_deploy.jar
Build time: Fri May 10 20:47:48 2019 (1557521268)
Build timestamp: 1557521268
Build timestamp as int: 1557521268

Rules version (SHA):

  0.29.0

Anything else relevant?
I have a few other deps that fail with the same error:

 @npm//node_modules/jsrsasign:jsrsasign.umd.js
 @npm//node_modules/sequelize-typescript:sequelize-typescript.umd.js
 @npm//node_modules/socket.io-client:socket.io-client.umd.js
 @npm//node_modules/@okta/okta-angular:okta-angular.umd.js

I also tried running the bowserify command on it manually like and it worked without issue

yarn browserify node_modules/crc/lib/index.js -p [ named-amd --name crc ] -s browserify_soa_crc -o crc.umd.js

[alexeagle]

I ran into this trying to upgrade tsickle:

browserify-wrapped failed: error executing command bazel-out/host/bin/external/build_bazel_rules_nodejs/internal/npm_install/browserify-wrapped tsickle core-util-is node_modules/core-util-is/lib/util.js ... (remaining 1 argument(s) skipped)

Use --sandbox_debug to see verbose messages from the sandbox
path.js:39
    throw new ERR_INVALID_ARG_TYPE('path', 'string', path);
    ^

[alexeagle]

Did a bit of debugging. Didn't get to the bottom of it so I'll record some notes before I forget:

• change the downstream project to use rules_nodejs from sources

local_repository(path="../rules_nodejs",#http_archive(
    name = "build_bazel_rules_nodejs",
    #sha256 = "e04a82a72146bfbca2d0575947daa60fda1878c8d3a3afe868a8ec39a6b968bb",
    #urls = ["https://github.com/bazelbuild/rules_nodejs/releases/download/0.31.1/rules_nodejs-0.31.1.tar.gz"],
)

load("@build_bazel_rules_nodejs//:package.bzl", "rules_nodejs_dev_dependencies")

• change rules_nodejs to use the non-minified bundle so you can debug: master...alexeagle:debugging

browserify is trying to resolve builtin modules like util. If you dump the options you see

opts.modules = {
  assert: 284,
  buffer: 178,
  child_process: 740,
  cluster: 740,
  console: 439,
  constants: 360,
  crypto: 159,
  dgram: 740,
  dns: 740,
  domain: 91,
  events: 730,
  fs: 740,
  http: 755,
  https: 196,
  inspector: 740,
  module: 740,
  net: 740,
  os: 591,
  path: 96,
  perf_hooks: 740,
  punycode: 116,
  querystring: 744,
  readline: 740,
  repl: 740,
  stream: 249,
  _stream_duplex: 494,
  _stream_passthrough: 676,
  _stream_readable: 285,
  _stream_transform: 920,
  _stream_writable: 487,
  string_decoder: 20,
  sys: 435,
  timers: 384,
  tls: 740,
  tty: 327,
  url: 10,
  util: 435,
  vm: 870,
  zlib: 305,
  _process: 937 }

and it's one of these being returned and then expected to be a string.

my guess is that the failing packages are those that do a require() of a builtin, and something about how we host it under Bazel prevents browserify from properly detecting these or resolving them differently

[Toxicable]

Found the issue here.
It's to do with how we've bundled up browserify for vendoring.

browserify has a list of builtin modules with browser equivilents.
So when looking at deps, if it finds a builtin like buffer it can replace it.

With npm browserify it has this mapping as a dictionary: {buffer: 'path/to/replacement.js'} but in our version those paths are replaced with webpack module id's so it's {builtin: 271}
This is where we get our error, expecting path, not number.

One solution here would be to vendor in a bunch more files instead of a webpacked version.
Another solution might be to tweak the webpack settings to somehow preserve those paths, not sure if this is possible though.

[alexeagle]

Sigh, I was too quick to cut the release last night. I tried this downstream in the incremental-dom project and there's another package that fails to browserify in another way

Gen
erated UMD bundle for sinon npm package [browserify] failed (Exi
t 1) browserify-wrapped failed: error executing command bazel-ou
t/host/bin/external/build_bazel_rules_nodejs/internal/npm_instal
l/browserify-wrapped incremental_dom sinon external/npm/node_mod
ules/sinon/lib/sinon.js ... (remaining 1 argument(s) skipped)

Use --sandbox_debug to see verbose messages from the sandbox
SyntaxError: 'import' and 'export' may appear only with 'sourceT
ype: module' (39:0) while parsing /home/alexeagle/.cache/bazel/_
bazel_alexeagle/752844a066d13a797cc6516bf45c9a78/sandbox/linux-s
andbox/1/execroot/incremental_dom/external/npm/node_modules/type
-detect/index.js while parsing file: /home/alexeagle/.cache/baze
l/_bazel_alexeagle/752844a066d13a797cc6516bf45c9a78/sandbox/linu
x-sandbox/1/execroot/incremental_dom/external/npm/node_modules/type-detect/index.js
    at o._flush (/home/alexeagle/.cache/bazel/_bazel_alexeagle/752844a066d13a797cc6516bf45c9a78/external/build_bazel_rules_nodejs/third_party/github.com/browserify/browserify/index.min.js:144:6353)

[Toxicable]

@alexeagle This PR owuld solve the import/export with the es6ify plugin #761

[alexeagle]

okay fixed at HEAD again, it works with incremental-dom