chore: code review feedback

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"],

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 [],

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",
     ],
 )
 

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');
+  });
 });

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"],
+)

packages/typescript/test/ts_project/empty_intermediate/c.ts

@@ -0,0 +1,3 @@
+import {a} from './a';
+
+console.log(a);

packages/typescript/test/ts_project/empty_intermediate/tsconfig-a.json

@@ -0,0 +1,6 @@
+{
+    "files": ["a.ts"],
+    "compilerOptions": {
+        "declaration": true
+    }
+}

packages/typescript/test/ts_project/empty_intermediate/tsconfig-b.json

@@ -0,0 +1,3 @@
+{
+    "files": []
+}

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",
+        ],
+    }
+}

packages/typescript/test/ts_project/simple/index.ts

@@ -0,0 +1 @@
+export const a: string = 'hello';