From d245d09c15e76f8db2937f6f2db74863da7bde24 Mon Sep 17 00:00:00 2001
From: Greg Magolan <gmagolan@gmail.com>
Date: Wed, 18 Dec 2019 21:31:46 -0800
Subject: [PATCH] refactor(builtin): add `args` to yarn_install & npm_install
 (#1462)

BREAKING CHANGE:
`args` in yarn_install and npm_install can be used to pass arbitrary arguments so we removed the following attributes:
* prod_only from yarn_install and npm_install; should be replaced by args = ["--prod"] and args = ["--production"] respectively
* frozen_lockfile from yarn_install; should be replaced by args = ["--frozen-lockfile"]
* network_timeout from yanr_install; should be replaced by args = ["--network_timeout", "<time in ms>"]
---
 e2e/packages/WORKSPACE                |  6 ++-
 e2e/packages/npm1/package.json        |  3 ++
 e2e/packages/npm2/package.json        |  3 ++
 e2e/packages/npm_determinism.spec.js  |  7 ++++
 e2e/packages/yarn1/package.json       |  3 ++
 e2e/packages/yarn2/package.json       |  3 ++
 e2e/packages/yarn_determinism.spec.js |  8 ++++
 internal/npm_install/npm_install.bzl  | 58 +++++++++++----------------
 8 files changed, 54 insertions(+), 37 deletions(-)

diff --git a/e2e/packages/WORKSPACE b/e2e/packages/WORKSPACE
index c1f9d641b2..eff4e97037 100644
--- a/e2e/packages/WORKSPACE
+++ b/e2e/packages/WORKSPACE
@@ -17,16 +17,16 @@ node_repositories()
 
 npm_install(
     name = "e2e_packages_npm_install",
+    args = ["--production"],
     data = ["//:postinstall.js"],
     package_json = "//:npm1/package.json",
     package_lock_json = "//:npm1/package-lock.json",
-    # Just here as a smoke test for this attribute
-    prod_only = True,
     symlink_node_modules = False,
 )
 
 npm_install(
     name = "e2e_packages_npm_install_duplicate_for_determinism_testing",
+    args = ["--production"],
     data = ["//:postinstall.js"],
     package_json = "//:npm2/package.json",
     package_lock_json = "//:npm2/package-lock.json",
@@ -35,6 +35,7 @@ npm_install(
 
 yarn_install(
     name = "e2e_packages_yarn_install",
+    args = ["--prod"],
     data = ["//:postinstall.js"],
     package_json = "//:yarn1/package.json",
     symlink_node_modules = False,
@@ -43,6 +44,7 @@ yarn_install(
 
 yarn_install(
     name = "e2e_packages_yarn_install_duplicate_for_determinism_testing",
+    args = ["--prod"],
     data = ["//:postinstall.js"],
     package_json = "//:yarn2/package.json",
     symlink_node_modules = False,
diff --git a/e2e/packages/npm1/package.json b/e2e/packages/npm1/package.json
index 5bff01fd1d..396c0aeb47 100644
--- a/e2e/packages/npm1/package.json
+++ b/e2e/packages/npm1/package.json
@@ -4,6 +4,9 @@
     "jsesc": "~1.2.0",
     "jasmine": "2.8.0"
   },
+  "devDependencies": {
+    "tmp": "0.1.0"
+  },
   "scripts": {
     "postinstall": "node ./postinstall.js"
   }
diff --git a/e2e/packages/npm2/package.json b/e2e/packages/npm2/package.json
index 5bff01fd1d..396c0aeb47 100644
--- a/e2e/packages/npm2/package.json
+++ b/e2e/packages/npm2/package.json
@@ -4,6 +4,9 @@
     "jsesc": "~1.2.0",
     "jasmine": "2.8.0"
   },
+  "devDependencies": {
+    "tmp": "0.1.0"
+  },
   "scripts": {
     "postinstall": "node ./postinstall.js"
   }
diff --git a/e2e/packages/npm_determinism.spec.js b/e2e/packages/npm_determinism.spec.js
index e7504730d6..a6e08dfdc5 100644
--- a/e2e/packages/npm_determinism.spec.js
+++ b/e2e/packages/npm_determinism.spec.js
@@ -8,6 +8,13 @@ const packageJsonPath2 = packageJsonPath.replace(
     '/e2e_packages_npm_install_duplicate_for_determinism_testing/', '/e2e_packages_npm_install/');
 const packageJson2 = JSON.parse(fs.readFileSync(packageJsonPath2));
 
+try {
+  require.resolve('tmp');
+  console.error(
+      'expected tmp to not be installed by npm as --production was passed via args in npm_install');
+  process.exitCode = 1;
+} catch (_) {
+}
 
 if (packageJsonPath === packageJsonPath2) {
   console.error('expected different json paths');
diff --git a/e2e/packages/yarn1/package.json b/e2e/packages/yarn1/package.json
index 5bff01fd1d..396c0aeb47 100644
--- a/e2e/packages/yarn1/package.json
+++ b/e2e/packages/yarn1/package.json
@@ -4,6 +4,9 @@
     "jsesc": "~1.2.0",
     "jasmine": "2.8.0"
   },
+  "devDependencies": {
+    "tmp": "0.1.0"
+  },
   "scripts": {
     "postinstall": "node ./postinstall.js"
   }
diff --git a/e2e/packages/yarn2/package.json b/e2e/packages/yarn2/package.json
index 5bff01fd1d..396c0aeb47 100644
--- a/e2e/packages/yarn2/package.json
+++ b/e2e/packages/yarn2/package.json
@@ -4,6 +4,9 @@
     "jsesc": "~1.2.0",
     "jasmine": "2.8.0"
   },
+  "devDependencies": {
+    "tmp": "0.1.0"
+  },
   "scripts": {
     "postinstall": "node ./postinstall.js"
   }
diff --git a/e2e/packages/yarn_determinism.spec.js b/e2e/packages/yarn_determinism.spec.js
index c2b6814dab..281a753d6d 100644
--- a/e2e/packages/yarn_determinism.spec.js
+++ b/e2e/packages/yarn_determinism.spec.js
@@ -8,6 +8,14 @@ const packageJsonPath2 = packageJsonPath.replace(
     '/e2e_packages_yarn_install_duplicate_for_determinism_testing/', '/e2e_packages_yarn_install/');
 const packageJson2 = JSON.parse(fs.readFileSync(packageJsonPath2));
 
+try {
+  require.resolve('tmp');
+  console.error(
+      'expected tmp to not be installed by yarn as --prod was passed via args in yarn_install');
+  process.exitCode = 1;
+} catch (_) {
+}
+
 if (packageJsonPath === packageJsonPath2) {
   console.error('expected different json paths');
   process.exitCode = 1;
diff --git a/internal/npm_install/npm_install.bzl b/internal/npm_install/npm_install.bzl
index 6ea88ac9af..a06d3f8d9a 100644
--- a/internal/npm_install/npm_install.bzl
+++ b/internal/npm_install/npm_install.bzl
@@ -26,6 +26,10 @@ load("//internal/common:os_name.bzl", "is_windows_os")
 load("//internal/node:node_labels.bzl", "get_node_label", "get_npm_label", "get_yarn_label")
 
 COMMON_ATTRIBUTES = dict(dict(), **{
+    "timeout": attr.int(
+        default = 3600,
+        doc = """Maximum duration of the package manager execution in seconds.""",
+    ),
     "always_hide_bazel_files": attr.bool(
         doc = """Always hide Bazel build files such as `BUILD` and BUILD.bazel` by prefixing them with `_`.
 
@@ -101,10 +105,6 @@ fine grained npm dependencies.
         mandatory = True,
         allow_single_file = True,
     ),
-    "prod_only": attr.bool(
-        default = False,
-        doc = "Don't install devDependencies",
-    ),
     "quiet": attr.bool(
         default = True,
         doc = "If stdout and stderr should be printed to the terminal.",
@@ -207,10 +207,7 @@ def _npm_install_impl(repository_ctx):
     is_windows_host = is_windows_os(repository_ctx)
     node = repository_ctx.path(get_node_label(repository_ctx))
     npm = get_npm_label(repository_ctx)
-    npm_args = ["install"]
-
-    if repository_ctx.attr.prod_only:
-        npm_args.append("--production")
+    npm_args = ["install"] + repository_ctx.attr.args
 
     # If symlink_node_modules is true then run the package manager
     # in the package.json folder; otherwise, run it in the root of
@@ -294,10 +291,11 @@ cd "{root}" && "{npm}" {npm_args}
 
 npm_install = repository_rule(
     attrs = dict(COMMON_ATTRIBUTES, **{
-        "timeout": attr.int(
-            default = 3600,
-            doc = """Maximum duration of the command "npm install" in seconds
-            (default is 3600 seconds).""",
+        "args": attr.string_list(
+            doc = """Arguments passed to npm install.
+
+See npm CLI docs https://docs.npmjs.com/cli/install.html for complete list of supported arguments.""",
+            default = [],
         ),
         "package_lock_json": attr.label(
             mandatory = True,
@@ -345,15 +343,8 @@ def _yarn_install_impl(repository_ctx):
         repository_ctx.path(yarn),
         "--cwd",
         root,
-        "--network-timeout",
-        str(repository_ctx.attr.network_timeout * 1000),  # in ms
     ]
 
-    if repository_ctx.attr.frozen_lockfile:
-        args.append("--frozen-lockfile")
-
-    if repository_ctx.attr.prod_only:
-        args.append("--prod")
     if not repository_ctx.attr.use_global_yarn_cache:
         args.extend(["--cache-folder", repository_ctx.path("_yarn_cache")])
     else:
@@ -365,6 +356,8 @@ def _yarn_install_impl(repository_ctx):
         # artifacts somewhere, so we rely on yarn to be correct.
         args.extend(["--mutex", "network"])
 
+    args.extend(repository_ctx.attr.args)
+
     repository_ctx.report_progress("Running yarn install on %s" % repository_ctx.attr.package_json)
     result = repository_ctx.execute(
         args,
@@ -381,23 +374,11 @@ def _yarn_install_impl(repository_ctx):
 
 yarn_install = repository_rule(
     attrs = dict(COMMON_ATTRIBUTES, **{
-        "timeout": attr.int(
-            default = 3600,
-            doc = """Maximum duration of the command "yarn install" in seconds
-            (default is 3600 seconds).""",
-        ),
-        "frozen_lockfile": attr.bool(
-            default = False,
-            doc = """Passes the --frozen-lockfile flag to prevent updating yarn.lock.
+        "args": attr.string_list(
+            doc = """Arguments passed to yarn install.
 
-Note that enabling this option will require that you run yarn outside of Bazel
-when making changes to package.json.
-""",
-        ),
-        "network_timeout": attr.int(
-            default = 300,
-            doc = """Maximum duration of a network request made by yarn in seconds
-            (default is 300 seconds).""",
+See yarn CLI docs https://yarnpkg.com/en/docs/cli/install for complete list of supported arguments.""",
+            default = [],
         ),
         "use_global_yarn_cache": attr.bool(
             default = True,
@@ -406,8 +387,15 @@ when making changes to package.json.
 The cache lets you avoid downloading packages multiple times.
 However, it can introduce non-hermeticity, and the yarn cache can
 have bugs.
+
 Disabling this attribute causes every run of yarn to have a unique
 cache_directory.
+
+If True, this rule will pass `--mutex network` to yarn to ensure that
+the global cache can be shared by parallelized yarn_install rules.
+
+If False, this rule will pass `--cache-folder /path/to/external/repository/__yarn_cache`
+to yarn so that the local cache is contained within the external repository.
 """,
         ),
         "yarn_lock": attr.label(