From ae1aa6a1e4b43af5310d1535cf34b24f1ce6f814 Mon Sep 17 00:00:00 2001 From: Tom Carrio Date: Mon, 1 Nov 2021 01:22:35 -0400 Subject: [PATCH] feat: support multiple node_modules roots in require_patch Refs: #266 --- WORKSPACE | 15 +++++++ internal/node/node.bzl | 5 ++- internal/node/require_patch.js | 39 +++++++++++++------ internal/node/test/BUILD.bazel | 15 ++++++- .../node/test/multi_module_resolution.spec.js | 7 ++++ tools/fine_grained_deps_yarn/package.json | 17 +------- tools/multi_root_deps_npm/package-lock.json | 19 +++++++++ tools/multi_root_deps_npm/package.json | 6 +++ tools/multi_root_deps_yarn/package.json | 6 +++ tools/multi_root_deps_yarn/yarn.lock | 15 +++++++ 10 files changed, 112 insertions(+), 32 deletions(-) create mode 100644 internal/node/test/multi_module_resolution.spec.js create mode 100644 tools/multi_root_deps_npm/package-lock.json create mode 100644 tools/multi_root_deps_npm/package.json create mode 100644 tools/multi_root_deps_yarn/package.json create mode 100644 tools/multi_root_deps_yarn/yarn.lock diff --git a/WORKSPACE b/WORKSPACE index fdaa5dcaf6..d637d2e248 100644 --- a/WORKSPACE +++ b/WORKSPACE @@ -506,6 +506,21 @@ filegroup( yarn_lock = "//:tools/fine_grained_goldens/yarn.lock", ) +yarn_install( + name = "multi_root_deps_yarn", + package_json = "//:tools/multi_root_deps_yarn/package.json", + symlink_node_modules = False, + yarn_lock = "//:tools/multi_root_deps_yarn/yarn.lock", +) + +npm_install( + name = "multi_root_deps_npm", + npm_command = "install", + package_json = "//:tools/multi_root_deps_npm/package.json", + package_lock_json = "//:tools/multi_root_deps_npm/package-lock.json", + symlink_node_modules = False, +) + # # RBE configuration # diff --git a/internal/node/node.bzl b/internal/node/node.bzl index a3bebbe31c..d9cf20f101 100644 --- a/internal/node/node.bzl +++ b/internal/node/node.bzl @@ -172,8 +172,9 @@ def _nodejs_binary_impl(ctx): node_modules_roots = _compute_node_modules_roots(ctx) - if "" in node_modules_roots: - node_modules_root = node_modules_roots[""] + "/node_modules" + node_modules_roots_key = node_modules_roots.keys() + if len(node_modules_roots_key) > 0: + node_modules_root = ":".join([root + "/node_modules" for root in node_modules_roots]) else: # there are no fine grained deps but we still need a node_modules_root even if it is a non-existant one node_modules_root = "build_bazel_rules_nodejs/node_modules" diff --git a/internal/node/require_patch.js b/internal/node/require_patch.js index e88833b737..8575623c53 100644 --- a/internal/node/require_patch.js +++ b/internal/node/require_patch.js @@ -39,6 +39,10 @@ function log_verbose(...m) { if (VERBOSE_LOGS) console.error(`[${path.basename(__filename)}]`, ...m); } +function pretty_json(m) { + return JSON.stringify(m, undefined, 2); +} + /** * The module roots as pairs of a RegExp to match the require path, and a * module_root to substitute for the require path. @@ -54,14 +58,23 @@ const BIN_DIR = 'TEMPLATED_bin_dir'; const GEN_DIR = 'TEMPLATED_gen_dir'; const TARGET = 'TEMPLATED_target'; +/** + * The node_modules roots as a list for importing modules from multiple roots. + * + * There is currently no support for duplicate modules across roots. Avoid + * including identical packages from multiple roots, such as + * ['@npm_a//jest', '@npm_b//jest'] + */ +const NODE_MODULES_ROOTS = NODE_MODULES_ROOT.split(":") + log_verbose(`patching require for ${TARGET} cwd: ${process.cwd()} RUNFILES: ${process.env.RUNFILES} TARGET: ${TARGET} BIN_DIR: ${BIN_DIR} GEN_DIR: ${GEN_DIR} - MODULE_ROOTS: ${JSON.stringify(MODULE_ROOTS, undefined, 2)} - NODE_MODULES_ROOT: ${NODE_MODULES_ROOT} + MODULE_ROOTS: ${pretty_json(MODULE_ROOTS)} + NODE_MODULES_ROOTS: ${pretty_json(NODE_MODULES_ROOT)} USER_WORKSPACE_NAME: ${USER_WORKSPACE_NAME} `); @@ -456,16 +469,18 @@ module.constructor._resolveFilename = } // If import was not resolved above then attempt to resolve - // within the node_modules filegroup in use - try { - const resolved = originalResolveFilename( - resolveRunfiles(undefined, NODE_MODULES_ROOT, request), parent, isMain, options); - log_verbose( - `resolved ${request} within node_modules (${NODE_MODULES_ROOT}) to ` + - `${resolved} from ${parentFilename}`); - return resolved; - } catch (e) { - failedResolutions.push(`node_modules attribute (${NODE_MODULES_ROOT}) - ${e.toString()}`); + // within the node_modules filegroups in use + for (const nodeModulesRoot of NODE_MODULES_ROOTS) { + try { + const resolved = originalResolveFilename( + resolveRunfiles(undefined, nodeModulesRoot, request), parent, isMain, options); + log_verbose( + `resolved ${request} within node_modules (${nodeModulesRoot}) to ` + + `${resolved} from ${parentFilename}`); + return resolved; + } catch (e) { + failedResolutions.push(`node_modules attribute (${nodeModulesRoot}) - ${e.toString()}`); + } } // Print the same error message that vanilla nodejs does. diff --git a/internal/node/test/BUILD.bazel b/internal/node/test/BUILD.bazel index 1bfe3a58d2..2f26923689 100644 --- a/internal/node/test/BUILD.bazel +++ b/internal/node/test/BUILD.bazel @@ -90,7 +90,7 @@ nodejs_test( ) nodejs_test( - name = "mode_resolution_test", + name = "module_resolution_test", data = [ "//internal/node/test/lib1", "//internal/node/test/lib2", @@ -209,7 +209,7 @@ write_file( ) nodejs_test( - name = "mode_resolution_built_test", + name = "module_resolution_built_test", data = [ "//internal/node/test/lib1", "//internal/node/test/lib2", @@ -226,6 +226,17 @@ nodejs_test( templated_args = ["--bazel_patch_module_resolver"], ) +nodejs_test( + name = "multi_module_resolution_built_test", + data = [ + "@multi_root_deps_npm//rxjs", + "@multi_root_deps_yarn//rxjs", + ], + entry_point = ":multi_module_resolution.spec.js", + # TODO: passes locally without this flag but fails on CircleCI + templated_args = ["--bazel_patch_module_resolver"], +) + nodejs_test( name = "data_resolution_built_test", data = [ diff --git a/internal/node/test/multi_module_resolution.spec.js b/internal/node/test/multi_module_resolution.spec.js new file mode 100644 index 0000000000..7413604b8d --- /dev/null +++ b/internal/node/test/multi_module_resolution.spec.js @@ -0,0 +1,7 @@ +// test node npm resolution with multiple roots +try { + require('rxjs'); +} catch (err) { + console.error('should resolve to one of the rxjs modules by default'); + process.exitCode = 1; +} diff --git a/tools/fine_grained_deps_yarn/package.json b/tools/fine_grained_deps_yarn/package.json index 75ff2055ac..3916ec0649 100644 --- a/tools/fine_grained_deps_yarn/package.json +++ b/tools/fine_grained_deps_yarn/package.json @@ -1,21 +1,6 @@ { - "description": "runtime dependencies fine_grained_deps e2e test", - "devDependencies": { - "jasmine": "3.3.0", - "jasmine-core": "3.3.0", - "typescript": "3.0.3" - }, + "description": "runtime dependencies multi_root_deps e2e test", "dependencies": { - "@gregmagolan/test-a": "0.0.4", - "@gregmagolan/test-b": "0.0.2", - "ajv": "5.5.2", - "chokidar": "2.0.4", - "http-server": "github:alexeagle/http-server#97205e945b69091606ed83aa0c8489e9ce65d282", - "klaw": "1.3.1", - "local-module": "link:../../tools/npm_packages/local_module/yarn", "rxjs": "6.5.0" - }, - "scripts": { - "postinstall": "node ../../internal/npm_install/test/postinstall.js" } } diff --git a/tools/multi_root_deps_npm/package-lock.json b/tools/multi_root_deps_npm/package-lock.json new file mode 100644 index 0000000000..437e25ef89 --- /dev/null +++ b/tools/multi_root_deps_npm/package-lock.json @@ -0,0 +1,19 @@ +{ + "requires": true, + "lockfileVersion": 1, + "dependencies": { + "rxjs": { + "version": "6.5.0", + "resolved": "https://registry.npmjs.org/rxjs/-/rxjs-6.5.0.tgz", + "integrity": "sha512-FOTfP3LK5VN3dDtr+wjFAeKVe5nTPPTC2+NUFJ8kyuO+YbIl/aME0eQDiX2MCVgnhKyuUYaEjgZEx8iL/4AV6A==", + "requires": { + "tslib": "^1.9.0" + } + }, + "tslib": { + "version": "1.10.0", + "resolved": "https://registry.npmjs.org/tslib/-/tslib-1.10.0.tgz", + "integrity": "sha512-qOebF53frne81cf0S9B41ByenJ3/IuH8yJKngAX35CmiZySA0khhkovshKK+jGCaMnVomla7gVlIcc3EvKPbTQ==" + } + } +} diff --git a/tools/multi_root_deps_npm/package.json b/tools/multi_root_deps_npm/package.json new file mode 100644 index 0000000000..3916ec0649 --- /dev/null +++ b/tools/multi_root_deps_npm/package.json @@ -0,0 +1,6 @@ +{ + "description": "runtime dependencies multi_root_deps e2e test", + "dependencies": { + "rxjs": "6.5.0" + } +} diff --git a/tools/multi_root_deps_yarn/package.json b/tools/multi_root_deps_yarn/package.json new file mode 100644 index 0000000000..3916ec0649 --- /dev/null +++ b/tools/multi_root_deps_yarn/package.json @@ -0,0 +1,6 @@ +{ + "description": "runtime dependencies multi_root_deps e2e test", + "dependencies": { + "rxjs": "6.5.0" + } +} diff --git a/tools/multi_root_deps_yarn/yarn.lock b/tools/multi_root_deps_yarn/yarn.lock new file mode 100644 index 0000000000..5737f10949 --- /dev/null +++ b/tools/multi_root_deps_yarn/yarn.lock @@ -0,0 +1,15 @@ +# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY. +# yarn lockfile v1 + + +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" + +tslib@^1.9.0: + version "1.14.1" + resolved "https://registry.yarnpkg.com/tslib/-/tslib-1.14.1.tgz#cf2d38bdc34a134bcaf1091c41f6619e2f672d00" + integrity sha512-Xni35NKzjgMrwevysHTCArtLDpPvye8zV/0E4EyYn43P7/7qvQwPh9BGkHewbMulVntbigmcT7rdX3BNo9wRJg==