refactor(builtin): remove node_modules attribute from nodejs_binary, …

…nodejs_test & ts_library

BREAKING CHANGE:

We removed the node_modules attribute from `nodejs_binary`, `nodejs_test`, `jasmine_node_test` & `ts_library`.

If you are using the `node_modules` attribute, you can simply add the target specified there to the `data` or `deps` attribute of the rule instead.

For example,

```
nodejs_test(
    name = "test",
    data = [
        "test.js",
        "@npm//:node_modules",
    ],
    entry_point = "test.js",
)
```

or

```
ts_library(
    name = "lib",
    srcs = glob(["*.ts"]),
    tsconfig = ":tsconfig.json",
    deps = ["@npm//:node_modules"],
)
```

We also dropped support for filegroup based node_modules target and removed `node_modules_filegroup` from `index.bzl`.

If you are using this feature for user-managed deps, you must now a `js_library` target
with `external_npm_package` set to `True` instead.

For example,

```
js_library(
    name = "node_modules",
    srcs = glob(
        include = [
            "node_modules/**/*.js",
            "node_modules/**/*.d.ts",
            "node_modules/**/*.json",
            "node_modules/.bin/*",
        ],
        exclude = [
            # Files under test & docs may contain file names that
            # are not legal Bazel labels (e.g.,
            # node_modules/ecstatic/test/public/中文/檔案.html)
            "node_modules/**/test/**",
            "node_modules/**/docs/**",
            # Files with spaces in the name are not legal Bazel labels
            "node_modules/**/* */**",
            "node_modules/**/* *",
        ],
    ),
    # Provide ExternalNpmPackageInfo which is used by downstream rules
    # that use these npm dependencies
    external_npm_package = True,
)

nodejs_test(
    name = "test",
    data = [
        "test.js",
        ":node_modules",
    ],
    entry_point = "test.js",
)
```

See `examples/user_managed_deps` for a working example of user-managed npm dependencies.

.bazelignore

@@ -1,7 +1,16 @@
-node_modules
-dist
+# NB: sematics here are not the same as .gitignore
+# see https://github.com/bazelbuild/bazel/issues/8106
+.git
 bazel-out
+
+# **/symlinked_node_modules_yarn
+node_modules
 e2e/symlinked_node_modules_yarn/node_modules
-e2e/symlinked_node_modules_npm/node_modules/
+e2e/symlinked_node_modules_npm/node_modules
 packages/angular/node_modules
 examples/angular/node_modules
+examples/user_managed_deps/node_modules
+
+# **/dist
+dist
+examples/user_managed_deps/dist

BUILD.bazel

@@ -57,13 +57,6 @@ bzl_library(
     ],
 )
 
-# Empty node_modules filegroup used for the default
-# value of the node_modules attribute in nodejs_binary
-filegroup(
-    name = "node_modules_none",
-    srcs = [],
-)
-
 # BEGIN-INTERNAL
 codeowners(
     pattern = "*",

WORKSPACE

@@ -53,22 +53,6 @@ yarn_install(
     environment = {
         "SOME_USER_ENV": "yarn is great!",
     },
-    # The @npm//:node_modules_filegroup generated by manual_build_file_contents
-    # is used in the //packages/typescript/test/reference_types_directive:tsconfig_types
-    # test. For now we're still supporting node_modules as a filegroup tho this may
-    # change in the future. The default generated //:node_modules target is a js_library
-    # rule which provides NpmPackageInfo but we have not yet dropped support for
-    # filegroup based node_modules target.
-    manual_build_file_contents = """
-filegroup(
-  name = "node_modules_filegroup",
-  srcs = [
-    "//@types/hammerjs:hammerjs__files",
-    "//@types/jasmine:jasmine__files",
-    "//typescript:typescript__files",
-  ],
-)
-""",
     package_json = "//:package.json",
     yarn_lock = "//:yarn.lock",
 )

e2e/bazel_managed_deps/BUILD.bazel

@@ -1,15 +1,5 @@
 load("@npm//@bazel/jasmine:index.bzl", "jasmine_node_test")
 
-# Test what happens when we depend on the catch-all "node_modules" rule rather
-# than declare our dependencies on individual npm packages.
-# This is the legacy behavior prior to 0.13, so it also proves backwards-compatibility.
-jasmine_node_test(
-    name = "test",
-    srcs = glob(["*.spec.js"]),
-    data = ["@npm//:bin_files"],
-    node_modules = "@npm//:node_modules",
-)
-
 # Test what happens when only certain NPM packages are in our dependencies.
 # These packages and their dependencies are copied to the execroot, but
 # the rest are not.

e2e/node_loader_no_preserve_symlinks/BUILD.bazel

@@ -6,8 +6,8 @@ nodejs_test(
     name = "test",
     data = [
         "node_loader_test.spec.js",
+        "@npm//:node_modules",
     ],
     entry_point = ":node_loader_test.spec.js",
-    node_modules = "@npm//:node_modules",
     templated_args = ["--nobazel_node_patches"],
 )

e2e/node_loader_preserve_symlinks/BUILD.bazel

@@ -6,9 +6,7 @@ nodejs_test(
     name = "test",
     data = [
         "node_loader_test.spec.js",
+        "@npm//:node_modules",
     ],
     entry_point = ":node_loader_test.spec.js",
-    node_modules = "@npm//:node_modules",
-    # legacy "node_modules" attribute means linker didn't see deps
-    templated_args = ["--bazel_patch_module_resolver"],
 )

e2e/packages/BUILD.bazel

@@ -17,9 +17,9 @@ VARIANTS = [
     data = [
         variant + ".spec.js",
         ":test_version",
+        "@e2e_packages_" + variant + "//:node_modules",
     ],
     entry_point = ":%s.spec.js" % variant,
-    node_modules = "@e2e_packages_" + variant + "//:node_modules",
     # On Windows, the yarn and npm variants fight over who creates the link
     # [link_node_modules.js] [Error: EEXIST: file already exists, mkdir 'C:\users\b\_bazel_b\p4se2lwa\execroot\e2e_packages\node_modules'] {
     # errno: -4075,
@@ -35,9 +35,9 @@ nodejs_test(
     data = [
         "npm_determinism.spec.js",
         "@e2e_packages_npm_install//:node_modules/jsesc/package.json",
+        "@e2e_packages_npm_install_duplicate_for_determinism_testing//:node_modules",
     ],
     entry_point = ":npm_determinism.spec.js",
-    node_modules = "@e2e_packages_npm_install_duplicate_for_determinism_testing//:node_modules",
     # TODO(gregmagolan): fix this test on windows
     tags = ["fix-windows"],
     # TODO: use runfiles
@@ -49,9 +49,9 @@ nodejs_test(
     data = [
         "yarn_determinism.spec.js",
         "@e2e_packages_yarn_install//:node_modules/jsesc/package.json",
+        "@e2e_packages_yarn_install_duplicate_for_determinism_testing//:node_modules",
     ],
     entry_point = ":yarn_determinism.spec.js",
-    node_modules = "@e2e_packages_yarn_install_duplicate_for_determinism_testing//:node_modules",
     # TODO(gregmagolan): fix this test on windows
     tags = ["fix-windows"],
     # TODO: use runfiles

examples/user_managed_deps/BUILD.bazel

@@ -1,30 +1,33 @@
-load("@build_bazel_rules_nodejs//:index.bzl", "nodejs_binary")
-load("@build_bazel_rules_nodejs//packages/jasmine:index.bzl", "jasmine_node_test")
+load("@build_bazel_rules_nodejs//:index.bzl", "js_library", "nodejs_binary", "nodejs_test")
 
-# Make the jasmine library available at runtime by exposing our node_modules
-# directory.
 # This is "user-managed" dependencies - Bazel knows nothing about our package manager.
 # We assume that developers will install the `node_modules` directory themselves and
 # keep it up-to-date.
-# This rule simply exposes files in the node_modules directory to Bazel so it can read
-# them as inputs to processes it spawns.
-filegroup(
+# This rule simply exposes files in the node_modules directory to Bazel using js_library
+# with external_npm_package set to True so it can read them as inputs to processes it spawns.
+js_library(
     name = "node_modules",
-    srcs = glob([
-        "node_modules/**/*.js",
-        "node_modules/**/*.d.ts",
-        "node_modules/**/*.json",
-    ]),
-)
-
-filegroup(
-    name = "decrement",
-    srcs = ["decrement.js"],
-)
-
-filegroup(
-    name = "program",
-    srcs = ["index.js"],
+    srcs = glob(
+        include = [
+            "node_modules/**/*.js",
+            "node_modules/**/*.d.ts",
+            "node_modules/**/*.json",
+            "node_modules/.bin/*",
+        ],
+        exclude = [
+            # Files under test & docs may contain file names that
+            # are not legal Bazel labels (e.g.,
+            # node_modules/ecstatic/test/public/中文/檔案.html)
+            "node_modules/**/test/**",
+            "node_modules/**/docs/**",
+            # Files with spaces in the name are not legal Bazel labels
+            "node_modules/**/* */**",
+            "node_modules/**/* *",
+        ],
+    ),
+    # Provide ExternalNpmPackageInfo which is used by downstream rules
+    # that use these npm dependencies
+    external_npm_package = True,
 )
 
 nodejs_binary(
@@ -38,15 +41,12 @@ nodejs_binary(
     entry_point = ":index.js",
 )
 
-jasmine_node_test(
+nodejs_test(
     name = "test",
-    srcs = glob(["*.spec.js"]),
-    node_modules = "//:node_modules",
-    tags = ["no-local-jasmine-deps"],
-    # user-managed deps don't expose the provider the linker needs to make require work
-    templated_args = ["--bazel_patch_module_resolver"],
-    deps = [
-        ":decrement",
-        ":program",
+    data = [
+        "decrement.js",
+        "index.js",
+        ":node_modules",
     ],
+    entry_point = "index.spec.js",
 )

examples/user_managed_deps/index.spec.js

@@ -1,14 +1,13 @@
+const _ = require('lodash');
 const {increment} = require('./index');
 const {decrement} = require('./decrement');
 
-describe('incrementing', () => {
-  it('should do that', () => {
-    expect(increment(1)).toBe(2);
-  });
-});
+if (!_.eq(increment(1), 2)) {
+  console.error('increment test failed');
+  process.exitCode = 1;
+}
 
-describe('decrementing', () => {
-  it('should do that', () => {
-    expect(decrement(1)).toBe(0);
-  });
-});
+if (!_.eq(decrement(1), 0)) {
+  console.error('decrement test failed');
+  process.exitCode = 1;
+}

examples/user_managed_deps/package.json

@@ -1,9 +1,7 @@
 {
   "description": "runtime dependencies for program example",
-  "devDependencies": {
-    "jasmine": "~3.4.0",
-    "jasmine-core": "~3.4.0",
-    "v8-coverage": "1.0.9"
+  "dependencies": {
+    "lodash": "4.17.20"
   },
   "scripts": {
     "pretest": "bazel run @nodejs//:yarn_node_repositories",