From 08b231aa7ed04af62eed276a82f2c0feaaec1242 Mon Sep 17 00:00:00 2001 From: Alex Eagle Date: Thu, 13 Jun 2019 17:13:20 -0700 Subject: [PATCH] fix(typescript): fix issue with types[] in non-sandboxed tsc To make worker mode work with TypeScript ambient type discovery, always zero out the compilerOptions#types in the generated tsconfig - users should provide these as deps[] instead. Fixes https://github.com/bazelbuild/rules_typescript/issues/449 --- common.bazelrc | 5 ---- e2e/ts_library/WORKSPACE | 9 +++++++ .../reference_types_directive/BUILD.bazel | 5 +++- .../tsconfig_types.ts | 3 +++ internal/npm_install/test/package/BUILD.bazel | 15 +++++++++++ packages/typescript/WORKSPACE | 2 +- packages/typescript/internal/build_defs.bzl | 27 ++++++++++++++++++- 7 files changed, 58 insertions(+), 8 deletions(-) create mode 100644 internal/npm_install/test/package/BUILD.bazel diff --git a/common.bazelrc b/common.bazelrc index 1ba1ea9748..f561c064ba 100644 --- a/common.bazelrc +++ b/common.bazelrc @@ -15,11 +15,6 @@ build --symlink_prefix=dist/ # build --symlink_prefix=/ # however this makes it harder to find outputs. -# This option is changed to true in Bazel 0.27 but exposes a known worker mode bug: -# https://github.com/bazelbuild/rules_typescript/issues/449 -# While we are fixing that issue, we need to keep this breaking change disabled -build --incompatible_list_based_execution_strategy_selection=false - # Specifies desired output mode for running tests. # Valid values are # 'summary' to output only test status summary diff --git a/e2e/ts_library/WORKSPACE b/e2e/ts_library/WORKSPACE index 503c271f9a..654764897c 100644 --- a/e2e/ts_library/WORKSPACE +++ b/e2e/ts_library/WORKSPACE @@ -26,6 +26,15 @@ load("@build_bazel_rules_nodejs//:defs.bzl", "yarn_install") yarn_install( name = "npm", + manual_build_file_contents = """ +filegroup( + name = "node_modules_filegroup", + srcs = [ + "//node_modules/@types/hammerjs:hammerjs__files", + "//node_modules/@types/jasmine:jasmine__files", + "//node_modules/typescript:typescript__files", + ], +)""", package_json = "//:package.json", yarn_lock = "//:yarn.lock", ) diff --git a/e2e/ts_library/reference_types_directive/BUILD.bazel b/e2e/ts_library/reference_types_directive/BUILD.bazel index bf4ceb5fb1..a984b223fe 100644 --- a/e2e/ts_library/reference_types_directive/BUILD.bazel +++ b/e2e/ts_library/reference_types_directive/BUILD.bazel @@ -7,7 +7,10 @@ ts_library( expected_diagnostics = [ "TS2304: Cannot find name 'Hammer'", ], - node_modules = "@npm//:node_modules", + # Use a filegroup to simulate user-managed node modules + # This exercises logic in build_defs.bzl that must preserve + # the types[] in tsconfig.json so that jasmine is found. + node_modules = "@npm//:node_modules_filegroup", tsconfig = ":tsconfig.json", ) diff --git a/e2e/ts_library/reference_types_directive/tsconfig_types.ts b/e2e/ts_library/reference_types_directive/tsconfig_types.ts index 657fce4f9b..da10367e55 100644 --- a/e2e/ts_library/reference_types_directive/tsconfig_types.ts +++ b/e2e/ts_library/reference_types_directive/tsconfig_types.ts @@ -1,3 +1,6 @@ +// Cecking that `describe` is a valid symbol here to ensure +// that @types/jasmine is getting picked up +describe('', () => {}); // Since "hammerjs" is not included in the types=[] array in // tsconfig, this should result in a compile error: TS2304: Cannot find name 'Hammer' console.log(typeof Hammer); diff --git a/internal/npm_install/test/package/BUILD.bazel b/internal/npm_install/test/package/BUILD.bazel new file mode 100644 index 0000000000..92b2d286be --- /dev/null +++ b/internal/npm_install/test/package/BUILD.bazel @@ -0,0 +1,15 @@ +# Generated file from yarn_install/npm_install rule. +# See $(bazel info output_base)/external/build_bazel_rules_nodejs/internal/npm_install/generate_build_file.js + +# All rules in other repositories can use these targets +package(default_visibility = ["//visibility:public"]) + +load("@build_bazel_rules_nodejs//internal/npm_install:node_module_library.bzl", "node_module_library") + +# The node_modules directory in one catch-all node_module_library. +# NB: Using this target may have bad performance implications if +# there are many files in target. +# See https://github.com/bazelbuild/bazel/issues/5153. +node_module_library( + name = "node_modules", +) diff --git a/packages/typescript/WORKSPACE b/packages/typescript/WORKSPACE index 75a999a003..98327f26ed 100644 --- a/packages/typescript/WORKSPACE +++ b/packages/typescript/WORKSPACE @@ -40,7 +40,7 @@ rules_nodejs_dev_dependencies() # With http_archive it only sees releases/download/*.tar.gz urls git_repository( name = "build_bazel_rules_typescript", - commit = "b1fa8ad9d15a4b46da800b33333512e538c69bd4", + commit = "78af714217bd2b844d2f75961263be4a9e39773e", remote = "http://github.com/bazelbuild/rules_typescript.git", ) diff --git a/packages/typescript/internal/build_defs.bzl b/packages/typescript/internal/build_defs.bzl index 632c35ed26..5f748f833b 100644 --- a/packages/typescript/internal/build_defs.bzl +++ b/packages/typescript/internal/build_defs.bzl @@ -23,6 +23,7 @@ load("@build_bazel_rules_typescript//internal:common/tsconfig.bzl", "create_tsco load("//internal:ts_config.bzl", "TsConfigInfo") _DEFAULT_COMPILER = "@npm//@bazel/typescript/bin:tsc_wrapped" +_DEFAULT_NODE_MODULES = Label("@npm//typescript:typescript__typings") def _trim_package_node_modules(package_name): # trim a package name down to its path prior to a node_modules @@ -35,6 +36,17 @@ def _trim_package_node_modules(package_name): segments += [n] return "/".join(segments) +# Detect use of bazel-managed node modules. +# Return True if the node_modules is the default or a course-grained deps +# `node_modules = "@npm//:node_modules"` +# Return False if the user used a hand-rolled filegroup +# `node_modules = "//:my_node_modules"` +# Note: this works for ts_library since we have a default for node_modules +# but wouldn't work for other rules like nodejs_binary +def _uses_bazel_managed_node_modules(ctx): + # If the user put a filegroup as the node_modules it will have no provider + return NodeModuleSources in ctx.attr.node_modules + # This function is similar but slightly different than _compute_node_modules_root # in /internal/node/node.bzl. TODO(gregmagolan): consolidate these functions def _compute_node_modules_root(ctx): @@ -206,6 +218,19 @@ def tsc_wrapped_tsconfig( # Since g3 isn't ready to do this yet config["compilerOptions"]["target"] = "es2015" + # It's fine for users to have types[] in their tsconfig.json to help the editor + # know which of the node_modules/@types/* entries to include in the program. + # But we don't want TypeScript to do any automatic resolution under tsc_wrapped + # because when not run in a sandbox, it will scan the @types directory and find + # entries that aren't in the action inputs. + # See https://github.com/bazelbuild/rules_typescript/issues/449 + # This setting isn't shared with g3 because there is no node_modules directory there. + # NB: Under user-managed dependencies we don't have a Provider in the node_modules + # so our mechanism to collect typings and put them into the program doesn't work. + # In that case we leave the types[] alone. + if _uses_bazel_managed_node_modules(ctx): + config["compilerOptions"]["types"] = [] + # If the user gives a tsconfig attribute, the generated file should extend # from the user's tsconfig. # See https://github.com/Microsoft/TypeScript/issues/9876 @@ -349,7 +374,7 @@ ts_library = rule( yarn_lock = "//:yarn.lock", ) """, - default = Label("@npm//typescript:typescript__typings"), + default = _DEFAULT_NODE_MODULES, ), "supports_workers": attr.bool( doc = """Intended for internal use only.