From af0229bba9c6191ec801bb28ad3de3db87c8144a Mon Sep 17 00:00:00 2001 From: Alex Eagle Date: Mon, 23 Mar 2020 22:35:31 -0700 Subject: [PATCH] chore: code review feedback --- examples/react_webpack/BUILD.bazel | 16 --- .../typescript/src/internal/ts_project.bzl | 117 +++++++++++++----- .../typescript/test/ts_project/b/BUILD.bazel | 3 + .../typescript/test/ts_project/b/b.spec.ts | 3 + .../ts_project/empty_intermediate/BUILD.bazel | 24 ++++ .../empty_intermediate/a.ts} | 0 .../test/ts_project/empty_intermediate/c.ts | 3 + .../empty_intermediate/tsconfig-a.json | 6 + .../empty_intermediate/tsconfig-b.json | 3 + .../empty_intermediate/tsconfig-c.json | 14 +++ .../simple}/BUILD.bazel | 0 .../simple}/index.golden.js | 0 .../test/ts_project/simple/index.ts | 1 + .../simple}/tsconfig.json | 0 14 files changed, 140 insertions(+), 50 deletions(-) create mode 100644 packages/typescript/test/ts_project/empty_intermediate/BUILD.bazel rename packages/typescript/test/{ts_project_simple/index.ts => ts_project/empty_intermediate/a.ts} (100%) create mode 100644 packages/typescript/test/ts_project/empty_intermediate/c.ts create mode 100644 packages/typescript/test/ts_project/empty_intermediate/tsconfig-a.json create mode 100644 packages/typescript/test/ts_project/empty_intermediate/tsconfig-b.json create mode 100644 packages/typescript/test/ts_project/empty_intermediate/tsconfig-c.json rename packages/typescript/test/{ts_project_simple => ts_project/simple}/BUILD.bazel (100%) rename packages/typescript/test/{ts_project_simple => ts_project/simple}/index.golden.js (100%) create mode 100644 packages/typescript/test/ts_project/simple/index.ts rename packages/typescript/test/{ts_project_simple => ts_project/simple}/tsconfig.json (100%) diff --git a/examples/react_webpack/BUILD.bazel b/examples/react_webpack/BUILD.bazel index 86bb368625..2a834eebd4 100644 --- a/examples/react_webpack/BUILD.bazel +++ b/examples/react_webpack/BUILD.bazel @@ -14,28 +14,12 @@ sass( ) ts_project( - name = "tsconfig", - srcs = [ - "index.tsx", - "types.d.ts", - ], - # The tsconfig.json file doesn't enable "declaration" or "composite" - # so tsc won't produce any .d.ts outputs. We need to tell Bazel not - # to expect those outputs being produced. - declaration = False, deps = [ "@npm//@types", "@npm//csstype", ], ) -# If you don't want to write //:tsconfig as the label for the TypeScript compilation, -# use an alias like this, so you can write //:compile_ts instead. -alias( - name = "compile_ts", - actual = "tsconfig", -) - webpack( name = "bundle", outs = ["app.bundle.js"], diff --git a/packages/typescript/src/internal/ts_project.bzl b/packages/typescript/src/internal/ts_project.bzl index 7fa53d5d20..681a875c89 100644 --- a/packages/typescript/src/internal/ts_project.bzl +++ b/packages/typescript/src/internal/ts_project.bzl @@ -2,14 +2,17 @@ load("@build_bazel_rules_nodejs//:providers.bzl", "DeclarationInfo", "NpmPackageInfo", "run_node") +_DEFAULT_TSC = "@npm//typescript/bin:tsc" + _ATTRS = { # NB: no restriction on extensions here, because tsc sometimes adds type-check support # for more file kinds (like require('some.json')) and also # if you swap out the `compiler` attribute (like with ngtsc) # that compiler might allow more sources than tsc does. "srcs": attr.label_list(allow_files = True, mandatory = True), + "args": attr.string_list(), "extends": attr.label_list(allow_files = [".json"]), - "tsc": attr.label(default = Label("@npm//typescript/bin:tsc"), executable = True, cfg = "host"), + "tsc": attr.label(default = Label(_DEFAULT_TSC), executable = True, cfg = "host"), "tsconfig": attr.label(mandatory = True, allow_single_file = [".json"]), "deps": attr.label_list(providers = [DeclarationInfo]), } @@ -35,6 +38,10 @@ _TsConfigInfo = provider( def _ts_project_impl(ctx): arguments = ctx.actions.args() + + # Add user specified arguments *before* rule supplied arguments + arguments.add_all(ctx.attr.args) + arguments.add_all([ "-p", ctx.file.tsconfig.short_path, @@ -71,7 +78,7 @@ def _ts_project_impl(ctx): # TODO: we could maybe filter these to be tsconfig.json or *.d.ts only # we don't expect tsc wants to read any other files from npm packages. deps_depsets.append(dep[NpmPackageInfo].sources) - elif DeclarationInfo in dep: + if DeclarationInfo in dep: deps_depsets.append(dep[DeclarationInfo].transitive_declarations) inputs = ctx.files.srcs + depset(transitive = deps_depsets).to_list() + [ctx.file.tsconfig] @@ -82,7 +89,21 @@ def _ts_project_impl(ctx): outputs.append(ctx.outputs.buildinfo_out) if len(outputs) == 0: - return [] + # no outputs, but still function as an intermediate rule in a chain like + # ts_project(a.ts) -> ts_project(no files) -> ts_project(b.ts) + return [ + DeclarationInfo( + transitive_declarations = depset(transitive = [ + dep[DeclarationInfo].transitive_declarations + for dep in ctx.attr.deps + ]), + ), + _TsConfigInfo(tsconfigs = depset(transitive = [ + dep[_TsConfigInfo].tsconfigs + for dep in ctx.attr.deps + if _TsConfigInfo in dep + ])), + ] run_node( ctx, @@ -104,6 +125,11 @@ def _ts_project_impl(ctx): for dep in ctx.attr.deps ]), ), + # DefaultInfo is what you see on the command-line for a built library, + # and determines what files are used by a simple non-provider-aware + # downstream library. + # Only the JavaScript outputs are intended for use in non-TS-aware + # dependents. DefaultInfo( files = runtime_files, runfiles = ctx.runfiles( @@ -130,6 +156,7 @@ def ts_project_macro( name = "tsconfig", tsconfig = None, srcs = None, + args = [], deps = [], extends = None, declaration = False, @@ -138,7 +165,7 @@ def ts_project_macro( composite = False, incremental = False, emit_declaration_only = False, - tsc = "@npm//typescript/bin:tsc", + tsc = _DEFAULT_TSC, **kwargs): """Compiles one TypeScript project using `tsc -p` @@ -147,6 +174,10 @@ def ts_project_macro( TODO(alexeagle): update https://github.com/bazelbuild/rules_nodejs/blob/master/docs/TypeScript.md#alternatives to describe the trade-offs between the two rules. + Some TypeScript options affect which files are emitted, and Bazel wants to know these ahead-of-time. + So several options from the tsconfig file must be mirrored as attributes to ts_project. + See https://www.typescriptlang.org/v2/en/tsconfig for a listing of the TypeScript options. + Any code that works with `tsc` should work with `ts_project` with a few caveats: - Bazel requires that the `outDir` (and `declarationDir`) be set to @@ -183,34 +214,49 @@ def ts_project_macro( > } > ``` - > Note: when using a non-sandboxed spawn strategy (which is the default on Windows), - > Bazel deletes outputs from the previous execution before running `tsc`. - > This causes a problem with TypeScript's incremental mode: if the `.tsbuildinfo` file - > is not known to be an output of the rule, then Bazel will leave it in the output - > directory, and when `tsc` runs, it may see that the outputs written by the prior - > invocation are up-to-date and skip the emit of these files. This will cause Bazel - > to intermittently fail with an error that some outputs were not written. - > This is why we depend on - > `composite` and/or `incremental` attributes to be provided, so we can tell Bazel to - > expect a `.tsbuildinfo` output to ensure it is deleted before a subsequent compilation. - > At present, we don't do anything useful with the `.tsbuildinfo` output, and this rule - > does not actually have incremental behavior. Deleting the file is actually - > counter-productive in terms of TypeScript compile performance. - > Follow https://github.com/bazelbuild/rules_nodejs/issues/1726 - - > Note: When using Project References, TypeScript will expect to verify that the outputs of referenced - > projects are up-to-date with respect to their inputs (this is true even without using the `--build` option). - > When using a non-sandboxed spawn strategy, `tsc` can read the sources from other `ts_project` - > rules in your project, and will expect that the `tsconfig.json` file for those references will - > indicate where the outputs were written. However the `outDir` is determined by this Bazel rule so - > it cannot be known from reading the `tsconfig.json` file. - > This problem is manifested as a TypeScript diagnostic like - > `error TS6305: Output file '/path/to/execroot/a.d.ts' has not been built from source file '/path/to/execroot/a.ts'.` - > As a workaround, you can give the Windows "fastbuild" output directory as the `outDir` in your tsconfig file. - > On other platforms, the value isn't read so it does no harm. - > See https://github.com/bazelbuild/rules_nodejs/tree/master/packages/typescript/test/ts_project as an example. - > We hope this will be fixed in a future release of TypeScript; - > follow https://github.com/microsoft/TypeScript/issues/37378 + ### Issues when running non-sandboxed + + When using a non-sandboxed spawn strategy (which is the default on Windows), you may + observe these problems which require workarounds: + + 1) Bazel deletes outputs from the previous execution before running `tsc`. + This causes a problem with TypeScript's incremental mode: if the `.tsbuildinfo` file + is not known to be an output of the rule, then Bazel will leave it in the output + directory, and when `tsc` runs, it may see that the outputs written by the prior + invocation are up-to-date and skip the emit of these files. This will cause Bazel + to intermittently fail with an error that some outputs were not written. + This is why we depend on `composite` and/or `incremental` attributes to be provided, + so we can tell Bazel to expect a `.tsbuildinfo` output to ensure it is deleted before a + subsequent compilation. + At present, we don't do anything useful with the `.tsbuildinfo` output, and this rule + does not actually have incremental behavior. Deleting the file is actually + counter-productive in terms of TypeScript compile performance. + Follow https://github.com/bazelbuild/rules_nodejs/issues/1726 + + 2) When using Project References, TypeScript will expect to verify that the outputs of referenced + projects are up-to-date with respect to their inputs. + (This is true even without using the `--build` option). + When using a non-sandboxed spawn strategy, `tsc` can read the sources from other `ts_project` + rules in your project, and will expect that the `tsconfig.json` file for those references will + indicate where the outputs were written. However the `outDir` is determined by this Bazel rule so + it cannot be known from reading the `tsconfig.json` file. + This problem is manifested as a TypeScript diagnostic like + `error TS6305: Output file '/path/to/execroot/a.d.ts' has not been built from source file '/path/to/execroot/a.ts'.` + As a workaround, you can give the Windows "fastbuild" output directory as the `outDir` in your tsconfig file. + On other platforms, the value isn't read so it does no harm. + See https://github.com/bazelbuild/rules_nodejs/tree/master/packages/typescript/test/ts_project as an example. + We hope this will be fixed in a future release of TypeScript; + follow https://github.com/microsoft/TypeScript/issues/37378 + + 3) When TypeScript encounters an import statement, it adds the source file resolved by that reference + to the program. However you may have included that source file in a different project, so this causes + the problem mentioned above where a source file is in multiple programs. + (Note, if you use Project References this is not the case, TS will know the referenced + file is part of the other program.) + This will result in duplicate emit for the same file, which produces an error + since the files written to the output tree are read-only. + A workaround for this is to add `--noResolve` to prevent TypeScript adding any files to the program + which weren't explicitly included by the tsconfig. Args: name: A name for the target. @@ -219,7 +265,7 @@ def ts_project_macro( srcs: List of labels of TypeScript source files to be provided to the compiler. - If absent, defaults to `**/*.ts` (all TypeScript files in the package). + If absent, defaults to `**/*.ts[x]` (all TypeScript files in the package). deps: List of labels of other rules that produce TypeScript typings (.d.ts files) @@ -231,6 +277,8 @@ def ts_project_macro( Must include any tsconfig files "chained" by extends clauses. + args: List of strings of additional command-line arguments to pass to tsc. + tsc: Label of the TypeScript compiler binary to run. Override this if your npm_install or yarn_install isn't named "npm" @@ -252,7 +300,7 @@ def ts_project_macro( """ if srcs == None: - srcs = native.glob(["**/*.ts"]) + srcs = native.glob(["**/*.ts", "**/*.tsx"]) if tsconfig == None: tsconfig = name + ".json" @@ -261,6 +309,7 @@ def ts_project_macro( name = name, srcs = srcs, deps = deps, + args = args, tsconfig = tsconfig, extends = extends, js_outs = _out_paths(srcs, ".js") if not emit_declaration_only else [], diff --git a/packages/typescript/test/ts_project/b/BUILD.bazel b/packages/typescript/test/ts_project/b/BUILD.bazel index 6f141bd59f..22304b501c 100644 --- a/packages/typescript/test/ts_project/b/BUILD.bazel +++ b/packages/typescript/test/ts_project/b/BUILD.bazel @@ -6,6 +6,8 @@ package(default_visibility = ["//packages/typescript/test:__subpackages__"]) ts_project( name = "tsconfig", # This will use ./tsconfig.json srcs = [":b.ts"], + # just a test for the pass-through args attribute + args = ["--emitBOM"], composite = True, extends = ["//packages/typescript/test/ts_project:tsconfig-base.json"], deps = ["//packages/typescript/test/ts_project/a:tsconfig"], @@ -20,6 +22,7 @@ ts_project( deps = [ ":tsconfig", "@npm//@types/jasmine", + "@npm//@types/node", ], ) diff --git a/packages/typescript/test/ts_project/b/b.spec.ts b/packages/typescript/test/ts_project/b/b.spec.ts index e72c0a1c3d..18fb545b46 100644 --- a/packages/typescript/test/ts_project/b/b.spec.ts +++ b/packages/typescript/test/ts_project/b/b.spec.ts @@ -7,4 +7,7 @@ describe('b', () => { sayHello(' world'); expect(captured).toBe('hello world'); }); + it('should include byte-order mark since that was passed in args attr', () => { + expect(require('fs').readFileSync(require.resolve('./b'), 'utf-8')[0]).toBe('\ufeff'); + }); }); diff --git a/packages/typescript/test/ts_project/empty_intermediate/BUILD.bazel b/packages/typescript/test/ts_project/empty_intermediate/BUILD.bazel new file mode 100644 index 0000000000..43bac3c113 --- /dev/null +++ b/packages/typescript/test/ts_project/empty_intermediate/BUILD.bazel @@ -0,0 +1,24 @@ +load("@npm_bazel_typescript//:index.bzl", "ts_project") + +ts_project( + name = "tsconfig-a", + srcs = ["a.ts"], + declaration = True, +) + +ts_project( + name = "tsconfig-b", + srcs = [], + deps = ["tsconfig-a"], +) + +ts_project( + name = "tsconfig-c", + srcs = ["c.ts"], + # Without --noResolve, tsc will see the import of ./a and add a.ts to the program + # It will then try to emit a.js which is an error since it was already written by + # tsconfig-a, and the file is read-only. + # Related: https://github.com/microsoft/TypeScript/issues/22208 + args = ["--noResolve"], + deps = ["tsconfig-b"], +) diff --git a/packages/typescript/test/ts_project_simple/index.ts b/packages/typescript/test/ts_project/empty_intermediate/a.ts similarity index 100% rename from packages/typescript/test/ts_project_simple/index.ts rename to packages/typescript/test/ts_project/empty_intermediate/a.ts diff --git a/packages/typescript/test/ts_project/empty_intermediate/c.ts b/packages/typescript/test/ts_project/empty_intermediate/c.ts new file mode 100644 index 0000000000..6431a4f84c --- /dev/null +++ b/packages/typescript/test/ts_project/empty_intermediate/c.ts @@ -0,0 +1,3 @@ +import {a} from './a'; + +console.log(a); diff --git a/packages/typescript/test/ts_project/empty_intermediate/tsconfig-a.json b/packages/typescript/test/ts_project/empty_intermediate/tsconfig-a.json new file mode 100644 index 0000000000..3b2c25a809 --- /dev/null +++ b/packages/typescript/test/ts_project/empty_intermediate/tsconfig-a.json @@ -0,0 +1,6 @@ +{ + "files": ["a.ts"], + "compilerOptions": { + "declaration": true + } +} \ No newline at end of file diff --git a/packages/typescript/test/ts_project/empty_intermediate/tsconfig-b.json b/packages/typescript/test/ts_project/empty_intermediate/tsconfig-b.json new file mode 100644 index 0000000000..6f5fba878a --- /dev/null +++ b/packages/typescript/test/ts_project/empty_intermediate/tsconfig-b.json @@ -0,0 +1,3 @@ +{ + "files": [] +} \ No newline at end of file diff --git a/packages/typescript/test/ts_project/empty_intermediate/tsconfig-c.json b/packages/typescript/test/ts_project/empty_intermediate/tsconfig-c.json new file mode 100644 index 0000000000..2d421c1d02 --- /dev/null +++ b/packages/typescript/test/ts_project/empty_intermediate/tsconfig-c.json @@ -0,0 +1,14 @@ +{ + "files": ["c.ts"], + "compilerOptions": { + "rootDirs": [ + ".", + "../../../../../bazel-out/darwin-fastbuild/bin/packages/typescript/test/ts_project/empty_intermediate", + "../../../../../bazel-out/k8-fastbuild/bin/packages/typescript/test/ts_project/empty_intermediate", + "../../../../../bazel-out/x64_windows-fastbuild/bin/packages/typescript/test/ts_project/empty_intermediate", + "../../../../../bazel-out/darwin-dbg/bin/packages/typescript/test/ts_project/empty_intermediate", + "../../../../../bazel-out/k8-dbg/bin/packages/typescript/test/ts_project/empty_intermediate", + "../../../../../bazel-out/x64_windows-dbg/bin/packages/typescript/test/ts_project/empty_intermediate", + ], + } +} \ No newline at end of file diff --git a/packages/typescript/test/ts_project_simple/BUILD.bazel b/packages/typescript/test/ts_project/simple/BUILD.bazel similarity index 100% rename from packages/typescript/test/ts_project_simple/BUILD.bazel rename to packages/typescript/test/ts_project/simple/BUILD.bazel diff --git a/packages/typescript/test/ts_project_simple/index.golden.js b/packages/typescript/test/ts_project/simple/index.golden.js similarity index 100% rename from packages/typescript/test/ts_project_simple/index.golden.js rename to packages/typescript/test/ts_project/simple/index.golden.js diff --git a/packages/typescript/test/ts_project/simple/index.ts b/packages/typescript/test/ts_project/simple/index.ts new file mode 100644 index 0000000000..a668b7e336 --- /dev/null +++ b/packages/typescript/test/ts_project/simple/index.ts @@ -0,0 +1 @@ +export const a: string = 'hello'; diff --git a/packages/typescript/test/ts_project_simple/tsconfig.json b/packages/typescript/test/ts_project/simple/tsconfig.json similarity index 100% rename from packages/typescript/test/ts_project_simple/tsconfig.json rename to packages/typescript/test/ts_project/simple/tsconfig.json