fix(builtin): fix for symlinked node_modules issue #802

This will fix the #802 case of deleting and manually re-creating node_modules by no longer writing any files under node_modules. A few keys caveats: 1. The downside of this is that the root @npm BUILD file grows huge again as all files under node_modules need to be added to exports_files([...]). If Bazel had a method to export all files without explicitly listing them such as exports_files_all() that would solve this problem. 2. npm package that ships with BUILD files would now cause issues with labels used by yarn_install & npm_install generated targets. With this change, packages such as @bazel/typescript which install bazel workspaces now ship with _BUILD.bazel files. The bazel workspaces installer is already looking for files of this name since the current repo rule renames BUILD files for the same reason. To solve (2), I propose publishing a tool that could be added as a postinstall step to rename all BUILD files under node_modules and prefix them with _. This should not be a common case as most npm packages don't ship with BUILD files but there are a few that do. For example, rxjs < 6.5.0 had build files for a few releases to support building it from source.
bazel-contrib · Jun 18, 2019 · 43cebe7 · 43cebe7
1 parent 08b231a
commit 43cebe7
Show file tree

Hide file tree

Showing 65 changed files with 10,556 additions and 6,044 deletions.
diff --git a/BUILD.bazel b/BUILD.bazel
@@ -38,6 +38,9 @@ npm_package(
         "defs.bzl",
         "package.bzl",
     ],
+    # Don't rename BUILD files as this package is not published to npm
+    # but is compressed in "release" below and published as a .tar.gz to GitHub
+    rename_build_files = False,
     # Don't replace the default 0.0.0-PLACEHOLDER for this npm_package since
     # we are packaging up the packager itself and this replacement will break it
     replace_with_version = "",

diff --git a/WORKSPACE b/WORKSPACE
@@ -169,15 +169,6 @@ filegroup(
     "//unidiff/bin:BUILD.bazel",
     "//unidiff:BUILD.bazel",
     "//zone.js:BUILD.bazel",
-    "//node_modules/@angular/core:BUILD.bazel",
-    "//node_modules/@gregmagolan:BUILD.bazel",
-    "//node_modules/@gregmagolan/test-a:BUILD.bazel",
-    "//node_modules/@gregmagolan/test-b:BUILD.bazel",
-    "//node_modules/ajv:BUILD.bazel",
-    "//node_modules/jasmine:BUILD.bazel",
-    "//node_modules/rxjs:BUILD.bazel",
-    "//node_modules/unidiff:BUILD.bazel",
-    "//node_modules/zone.js:BUILD.bazel",
   ],
 )""",
     package_json = "//internal/npm_install/test:package.json",

diff --git a/docs/node/node.html b/docs/node/node.html
@@ -369,7 +369,7 @@ <h3 id="nodejs_binary_args">Attributes</h3>
     ```
     nodejs_binary(
         name = "history-server",
-        entry_point = "@npm//node_modules/history-server:modules/cli.js",
+        entry_point = "@npm//:node_modules/history-server/modules/cli.js",
         data = ["@npm//history-server"],
     )
     ```
@@ -591,7 +591,7 @@ <h3 id="nodejs_test_args">Attributes</h3>
     ```
     nodejs_binary(
         name = "history-server",
-        entry_point = "@npm//node_modules/history-server:modules/cli.js",
+        entry_point = "@npm//:node_modules/history-server/modules/cli.js",
         data = ["@npm//history-server"],
     )
     ```

diff --git a/e2e/bazel_bin/test.sh b/e2e/bazel_bin/test.sh
@@ -27,7 +27,7 @@ else
 fi
 # --- end runfiles.bash initialization ---
 
-readonly OUT=$($(rlocation "npm/node_modules/testy/testy__bin"))
+readonly OUT=$($(rlocation "npm/testy/bin/testy"))
 
 if [ "$OUT" != "Hello world" ]; then
   echo "Expected output 'Hello world' but was $OUT"

diff --git a/e2e/karma_typescript/BUILD.bazel b/e2e/karma_typescript/BUILD.bazel
@@ -72,7 +72,7 @@ filegroup(
     name = "rxjs_umd_modules",
     srcs = [
         # do not sort
-        "@npm//node_modules/rxjs:bundles/rxjs.umd.js",
+        "@npm//:node_modules/rxjs/bundles/rxjs.umd.js",
         ":rxjs_shims.js",
     ],
 )

diff --git a/e2e/karma_typescript/package.json b/e2e/karma_typescript/package.json
@@ -4,7 +4,7 @@
     "@bazel/typescript": "file:../../dist/npm_bazel_typescript",
     "@types/jasmine": "^3.3.12",
     "jasmine": "^3.4.0",
-    "rxjs": "^6.4.0",
+    "rxjs": "6.5.0",
     "typescript": "2.9.2"
   },
   "scripts": {

diff --git a/e2e/ts_devserver/BUILD.bazel b/e2e/ts_devserver/BUILD.bazel
@@ -30,7 +30,7 @@ ts_devserver(
     entry_module = "e2e_ts_devserver/app",
     port = 8080,
     scripts = [
-        "@npm//node_modules/date-fns:date-fns.umd.js",
+        "@npm//date-fns:date-fns.umd.js",
     ],
     # We'll collect all the devmode JS sources from these TypeScript libraries
     deps = [":app"],

diff --git a/e2e/ts_library/WORKSPACE b/e2e/ts_library/WORKSPACE
@@ -30,9 +30,9 @@ yarn_install(
 filegroup(
   name = "node_modules_filegroup",
   srcs = [
-    "//node_modules/@types/hammerjs:hammerjs__files",
-    "//node_modules/@types/jasmine:jasmine__files",
-    "//node_modules/typescript:typescript__files",
+    "//@types/hammerjs:hammerjs__files",
+    "//@types/jasmine:jasmine__files",
+    "//typescript:typescript__files",
   ],
 )""",
     package_json = "//:package.json",

diff --git a/e2e/ts_library/googmodule/BUILD.bazel b/e2e/ts_library/googmodule/BUILD.bazel
@@ -13,7 +13,7 @@ nodejs_binary(
         "@npm//@bazel/typescript",
         "@npm//tsickle",
     ],
-    entry_point = "@npm//node_modules/@bazel/typescript:internal/tsc_wrapped/tsc_wrapped.js",
+    entry_point = "@npm//:node_modules/@bazel/typescript/internal/tsc_wrapped/tsc_wrapped.js",
     install_source_map_support = False,
 )
 

diff --git a/examples/protocol_buffers/BUILD.bazel b/examples/protocol_buffers/BUILD.bazel
@@ -85,8 +85,8 @@ rollup_bundle(
 genrule(
     name = "protobufjs",
     srcs = [
-        "@build_bazel_rules_typescript_protobufs_compiletime_deps//node_modules/protobufjs:dist/minimal/protobuf.min.js",
-        "@build_bazel_rules_typescript_protobufs_compiletime_deps//node_modules/long:dist/long.js",
+        "@build_bazel_rules_typescript_protobufs_compiletime_deps//:node_modules/protobufjs/dist/minimal/protobuf.min.js",
+        "@build_bazel_rules_typescript_protobufs_compiletime_deps//:node_modules/long/dist/long.js",
     ],
     outs = [
         "protobuf.min.js",

diff --git a/internal/e2e/fine_grained_deps/npm/package-lock.json b/internal/e2e/fine_grained_deps/npm/package-lock.json
diff --git a/internal/e2e/fine_grained_deps/npm/package.json b/internal/e2e/fine_grained_deps/npm/package.json
@@ -11,6 +11,6 @@
     "chokidar": "2.0.4",
     "http-server": "github:alexeagle/http-server#97205e945b69091606ed83aa0c8489e9ce65d282",
     "klaw": "1.3.1",
-    "rxjs": "6.3.3"
+    "rxjs": "6.5.0"
   }
 }
diff --git a/internal/e2e/fine_grained_deps/yarn/package.json b/internal/e2e/fine_grained_deps/yarn/package.json
@@ -11,6 +11,6 @@
     "chokidar": "2.0.4",
     "http-server": "github:alexeagle/http-server#97205e945b69091606ed83aa0c8489e9ce65d282",
     "klaw": "1.3.1",
-    "rxjs": "6.3.3"
+    "rxjs": "6.5.0"
   }
 }
diff --git a/internal/e2e/fine_grained_deps/yarn/yarn.lock b/internal/e2e/fine_grained_deps/yarn/yarn.lock
@@ -1121,10 +1121,10 @@ rimraf@^2.6.1:
   dependencies:
     glob "^7.0.5"
 
-rxjs@6.3.3:
-  version "6.3.3"
-  resolved "https://registry.yarnpkg.com/rxjs/-/rxjs-6.3.3.tgz#3c6a7fa420e844a81390fb1158a9ec614f4bad55"
-  integrity sha512-JTWmoY9tWCs7zvIk/CvRjhjGaOd+OVBM987mxFo+OW66cGpdKjZcpmc74ES1sB//7Kl/PAe8+wEakuhG4pcgOw==
+rxjs@6.5.0:
+  version "6.5.0"
+  resolved "https://registry.yarnpkg.com/rxjs/-/rxjs-6.5.0.tgz#1e4bc933f14a0c5c2ce7bb71ddd4244d42f4b04b"
+  integrity sha512-FOTfP3LK5VN3dDtr+wjFAeKVe5nTPPTC2+NUFJ8kyuO+YbIl/aME0eQDiX2MCVgnhKyuUYaEjgZEx8iL/4AV6A==
   dependencies:
     tslib "^1.9.0"
 

diff --git a/internal/e2e/packages/BUILD.bazel b/internal/e2e/packages/BUILD.bazel
@@ -22,13 +22,13 @@ VARIANTS = [
 jasmine_node_test(
     name = "npm_determinism_test",
     srcs = ["npm_determinism.spec.js"],
-    data = ["@internal_e2e_packages_npm_install//node_modules/jsesc:package.json"],
+    data = ["@internal_e2e_packages_npm_install//:node_modules/jsesc/package.json"],
     node_modules = "@internal_e2e_packages_npm_install_duplicate_for_determinism_testing//:node_modules",
 )
 
 jasmine_node_test(
     name = "yarn_determinism_test",
     srcs = ["yarn_determinism.spec.js"],
-    data = ["@internal_e2e_packages_yarn_install//node_modules/jsesc:package.json"],
+    data = ["@internal_e2e_packages_yarn_install//:node_modules/jsesc/package.json"],
     node_modules = "@internal_e2e_packages_yarn_install_duplicate_for_determinism_testing//:node_modules",
 )
diff --git a/internal/history-server/history_server.bzl b/internal/history-server/history_server.bzl
@@ -26,7 +26,7 @@ def history_server(templated_args = [], **kwargs):
 
     nodejs_binary_macro(
         node_modules = "@history-server_runtime_deps//:node_modules",
-        entry_point = "@history-server_runtime_deps//node_modules/history-server:modules/cli.js",
+        entry_point = "@history-server_runtime_deps//:node_modules/history-server/modules/cli.js",
         install_source_map_support = False,
         templated_args = templated_args,
         **kwargs

diff --git a/internal/http-server/http_server.bzl b/internal/http-server/http_server.bzl
@@ -29,7 +29,7 @@ def http_server(templated_args = [], **kwargs):
 
     nodejs_binary_macro(
         node_modules = "@http-server_runtime_deps//:node_modules",
-        entry_point = "@http-server_runtime_deps//node_modules/http-server:bin/http-server",
+        entry_point = "@http-server_runtime_deps//:node_modules/http-server/bin/http-server",
         install_source_map_support = False,
         templated_args = templated_args,
         **kwargs

diff --git a/internal/node/node.bzl b/internal/node/node.bzl
@@ -46,14 +46,17 @@ def _compute_node_modules_root(ctx):
       The node_modules root as a string
     """
     node_modules_root = None
-    if ctx.files.node_modules:
-        # ctx.files.node_modules is not an empty list
-        workspace = ctx.attr.node_modules.label.workspace_root.split("/")[1] if ctx.attr.node_modules.label.workspace_root else ctx.workspace_name
-        node_modules_root = "/".join([f for f in [
-            workspace,
-            _trim_package_node_modules(ctx.attr.node_modules.label.package),
-            "node_modules",
-        ] if f])
+    if ctx.attr.node_modules:
+        if NodeModuleSources in ctx.attr.node_modules:
+            node_modules_root = "/".join([ctx.attr.node_modules[NodeModuleSources].workspace, "node_modules"])
+        elif ctx.files.node_modules:
+            # ctx.files.node_modules is not an empty list
+            workspace = ctx.attr.node_modules.label.workspace_root.split("/")[1] if ctx.attr.node_modules.label.workspace_root else ctx.workspace_name
+            node_modules_root = "/".join([f for f in [
+                workspace,
+                _trim_package_node_modules(ctx.attr.node_modules.label.package),
+                "node_modules",
+            ] if f])
     for d in ctx.attr.data:
         if NodeModuleSources in d:
             possible_root = "/".join([d[NodeModuleSources].workspace, "node_modules"])
@@ -275,7 +278,7 @@ _NODEJS_EXECUTABLE_ATTRS = {
         ```
         nodejs_binary(
             name = "history-server",
-            entry_point = "@npm//node_modules/history-server:modules/cli.js",
+            entry_point = "@npm//:node_modules/history-server/modules/cli.js",
             data = ["@npm//history-server"],
         )
         ```