feat(typescript): add ts_project rule #1710

alexeagle · 2020-03-16T05:09:31Z

This is a very thin layer on top of vanilla tsc

examples/react_webpack/BUILD.bazel

packages/typescript/src/internal/ts_project.bzl

mrmeku · 2020-03-16T16:38:20Z

packages/typescript/src/internal/ts_project.bzl

+        progress_message = "Compiling TypeScript project %s" % ctx.file.tsconfig.short_path,
+    )
+
+    runtime_files = depset(ctx.outputs.js_outs + ctx.outputs.map_outs)


I'm not sure if this is important by if a src file is a .js dependency might it also need to be included as a runtime file?

If you have .js inputs, does tsc write them to the our dir? or does it only emit diagnostics?

yeah this needs a test case for .js inputs...

Toxicable

Excited to see this going in!
Also nice to see it can work as a stand alone tsc compiler - no project references

Toxicable · 2020-03-16T21:26:58Z

packages/typescript/src/internal/ts_project.bzl

+
+    if srcs == None:
+        srcs = native.glob(["**/*.ts"])
+    tsconfig = name + ".json"


Not sure if im a fan of this.
Could we possibly change the logic to default to tsconfig = ctx.testonly ? 'tsconfig-test.json' : 'tsconfig.json'.
But override with the tsconfig speciffied by the user if exists.
Usually we'd have libs where their "primary target" is a ts_library and this will use the same name as the dir, making it easier to reference.
This here prevents using a name that's different from your tsconfig, right?
Is there another reason why this might be a good idea?

I'll add a comment in the code explaining this.

Bazel's `name` property is arbitrarily controlled by the user. But here we constrain it to match the tsconfig file. The reason for doing this is so that BUILD file generation can be trivial. If you know the path of a referenced tsconfig file, you know what label to use to include it in the deps of another rule. If we allowed users to control it, then BUILD file generation would need heuristic semantics to do bazel query and try to figure out which label gives a target that is usable as a dep.

And on the rule docs

You can use a Bazel alias() to give an additional label for this typescript project compilation

Also note you could import the raw ts_project rule from the internal path, which I think we should leave undocumented, but it gives you the escape valve to avoid this macro logic.

Ah yeah I see the constraint here, sounds good to me with the extra docs and code gen considered

Discussed with @gregmagolan - even if users can control tsconfig and name attributes independently, it's still pretty easy for the generator to print all ts_project rules in the referenced package to find the right one. So tsconfig is now independent (though we recommend naming it the basename of the tsconfig.json and that default is convenient)

packages/typescript/src/internal/ts_project.bzl

Toxicable · 2020-03-16T21:33:14Z

packages/typescript/src/internal/ts_project.bzl

+    # ts, tsx, js, jsx, json
+    "srcs": attr.label_list(allow_files = True),
+    "extends": attr.label_list(allow_files = [".json"]),
+    "tsc": attr.label(default = Label("@npm//typescript/bin:tsc"), executable = True, cfg = "host"),


i'd love to see if this works by replacing this with ngtsc

yeah I played with it briefly and something didn't work, please give that a try if you have time :)

examples/react_webpack/BUILD.bazel

examples/react_webpack/tsconfig.json

packages/typescript/src/internal/ts_project.bzl

Toxicable

LGTM!

alexeagle · 2020-03-21T17:56:49Z

/cc @tjgq @evmar

evmar

I don't really have any context here. Why this in addition to a tsc rule that is also different than ts_library (?)

evmar · 2020-03-22T01:15:28Z

packages/typescript/src/internal/ts_project.bzl

+            "--diagnostics",
+            "--extendedDiagnostics",
+            # Prefer to show gory details rather than save space in console
+            "--noErrorTruncation",


We recently discovered that tsc can OOM when you set this flag, because without error truncation error generation can infinite loop.

good tip, thanks. I only set this here from reading through the tsc docs and it sounded helpful, will remove

Oh wait they fixed it by actually limiting it with the flag, heh microsoft/TypeScript#37461

This is a very thin layer on top of vanilla tsc

this fixes a bug on Windows where Bazel deletes known outputs like .js but then tsc doesn't write the .js file again on next action because the tsbuildinfo file indicates it is up-to-date

gregmagolan · 2020-03-24T02:44:04Z

examples/react_webpack/BUILD.bazel

+# 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",


nit: use tsconfig attribute and use "compile" as ts_project name to avoid alias?

oops, this BUILD file should have been updated for the change of declaration default to False too. will boil it down to the minimum

gregmagolan · 2020-03-24T02:51:03Z

packages/typescript/src/internal/ts_project.bzl

+
+load("@build_bazel_rules_nodejs//:providers.bzl", "DeclarationInfo", "NpmPackageInfo", "run_node")
+
+_ATTRS = {


I think we should add "args" in general to rules to allow users to pass their own arguments as

config files do not always support all arguments. I recently added "args" to rollup for this reason so
Angular could pass --silent which is command line only

users may want to override config file settings with command line arguments in some cases
General pattern is

"args": attr.string_list( doc = """Command line arguments to pass to <tool name>. Can be used to override config file settings.

and

# See CLI documentation at https://rollupjs.org/guide/en/#command-line-reference args = ctx.actions.args() # Add user specified arguments *before* rule supplied arguments args.add_all(ctx.attr.args)

where `args` is passed to `run_node`

added args pass-through with a delightful test coverage

gregmagolan · 2020-03-24T02:54:01Z

packages/typescript/src/internal/ts_project.bzl

+    # that compiler might allow more sources than tsc does.
+    "srcs": attr.label_list(allow_files = True, mandatory = True),
+    "extends": attr.label_list(allow_files = [".json"]),
+    "tsc": attr.label(default = Label("@npm//typescript/bin:tsc"), executable = True, cfg = "host"),


nit: set DEFAULT_COMPILER = "@npm//typescript/bin:tsc" as the string is in two places

It controls writing the .tsbuildinfo output

gregmagolan · 2020-03-24T04:55:40Z

packages/typescript/src/internal/ts_project.bzl

+            For example, `tsc = "@my_deps//typescript/bin:tsc"`
+            Or you can pass a custom compiler binary instead.
+
+        declaration: if the `declaration` bit is set in the tsconfig.


nit: links to typescript docs for these 1:1 mappings for more info?

added above

gregmagolan

🌮 ^ 🌮

gregmagolan · 2020-03-24T05:01:58Z

packages/typescript/src/internal/ts_project.bzl

+                for dep in ctx.attr.deps
+            ]),
+        ),
+        DefaultInfo(


nit: short comment on rationale for only including runtime files in DefaultInfo?

# 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.

gregmagolan · 2020-03-24T05:06:28Z

packages/typescript/src/internal/ts_project.bzl

+            # 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:


should this just be an if? They seem orthogonal and should overlap anyway.

hmm, I think this is true. Need to verify if we have coverage for it

gregmagolan · 2020-03-24T05:07:53Z

packages/typescript/src/internal/ts_project.bzl

+        outputs.append(ctx.outputs.buildinfo_out)
+
+    if len(outputs) == 0:
+        return []


should still return transitive declarations & tsconfigs?

yes, took a while to add a test case to expose this...

Also add a test for an intermediate ts_project rule with no srcs

alexeagle requested review from gregmagolan and soldair as code owners March 16, 2020 05:09

googlebot added the cla: yes label Mar 16, 2020

alexeagle force-pushed the ts_project branch from d0562be to a378a72 Compare March 16, 2020 05:52

mrmeku reviewed Mar 16, 2020

View reviewed changes

Toxicable reviewed Mar 16, 2020

View reviewed changes

alexeagle force-pushed the ts_project branch 2 times, most recently from 1b28486 to 91f43ab Compare March 17, 2020 01:44

Toxicable reviewed Mar 17, 2020

View reviewed changes

examples/react_webpack/tsconfig.json Outdated Show resolved Hide resolved

Toxicable reviewed Mar 17, 2020

View reviewed changes

packages/typescript/src/internal/ts_project.bzl Show resolved Hide resolved

alexeagle force-pushed the ts_project branch from 91f43ab to 3626565 Compare March 19, 2020 21:47

Toxicable approved these changes Mar 20, 2020

View reviewed changes

alexeagle force-pushed the ts_project branch from 3626565 to ff15767 Compare March 21, 2020 17:44

alexeagle force-pushed the ts_project branch from ff15767 to c5b6ed7 Compare March 22, 2020 00:58

evmar reviewed Mar 22, 2020

View reviewed changes

alexeagle force-pushed the ts_project branch 3 times, most recently from 68368c4 to b4eb2e0 Compare March 22, 2020 02:48

feat(typescript): add ts_project rule

d4f7750

This is a very thin layer on top of vanilla tsc

alexeagle force-pushed the ts_project branch from b4eb2e0 to d4f7750 Compare March 22, 2020 02:53

fixup! feat(typescript): add ts_project rule

71e77d9

alexeagle force-pushed the ts_project branch 2 times, most recently from cee6eb2 to d738f42 Compare March 23, 2020 04:34

fix(typescript): declare the .tsbuildinfo output

fc00d38

this fixes a bug on Windows where Bazel deletes known outputs like .js but then tsc doesn't write the .js file again on next action because the tsbuildinfo file indicates it is up-to-date

alexeagle force-pushed the ts_project branch from d738f42 to fc00d38 Compare March 23, 2020 04:38

chore: set ts_project#declaration default to False

147968b

gregmagolan reviewed Mar 24, 2020

View reviewed changes

chore: add incremental param to ts_project macro

2895333

It controls writing the .tsbuildinfo output

gregmagolan reviewed Mar 24, 2020

View reviewed changes

gregmagolan approved these changes Mar 24, 2020

View reviewed changes

gregmagolan reviewed Mar 24, 2020

View reviewed changes

alexeagle force-pushed the ts_project branch 3 times, most recently from af0229b to adb54d1 Compare March 24, 2020 22:10

chore: add args attribute to ts_project

a2d8735

Also add a test for an intermediate ts_project rule with no srcs

alexeagle force-pushed the ts_project branch from adb54d1 to a2d8735 Compare March 24, 2020 22:51

alexeagle merged commit 26f6698 into bazel-contrib:master Mar 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(typescript): add ts_project rule #1710

feat(typescript): add ts_project rule #1710

alexeagle commented Mar 16, 2020

mrmeku Mar 16, 2020

Toxicable Mar 16, 2020

alexeagle Mar 16, 2020

Toxicable left a comment

Toxicable Mar 16, 2020

alexeagle Mar 16, 2020

alexeagle Mar 16, 2020

Toxicable Mar 16, 2020

alexeagle Mar 23, 2020

Toxicable Mar 16, 2020

alexeagle Mar 16, 2020

Toxicable left a comment

alexeagle commented Mar 21, 2020

evmar left a comment

evmar Mar 22, 2020

alexeagle Mar 22, 2020

evmar Mar 22, 2020

gregmagolan Mar 24, 2020

alexeagle Mar 24, 2020

gregmagolan Mar 24, 2020 •

edited

Loading

alexeagle Mar 24, 2020

gregmagolan Mar 24, 2020

gregmagolan Mar 24, 2020

alexeagle Mar 24, 2020

gregmagolan left a comment

gregmagolan Mar 24, 2020

alexeagle Mar 24, 2020 •

edited

Loading

gregmagolan Mar 24, 2020

gregmagolan Mar 24, 2020

alexeagle Mar 24, 2020

gregmagolan Mar 24, 2020

alexeagle Mar 24, 2020


		load("@build_bazel_rules_nodejs//:providers.bzl", "DeclarationInfo", "NpmPackageInfo", "run_node")

		_ATTRS = {

feat(typescript): add ts_project rule #1710

feat(typescript): add ts_project rule #1710

Conversation

alexeagle commented Mar 16, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Toxicable left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Toxicable left a comment

Choose a reason for hiding this comment

alexeagle commented Mar 21, 2020

evmar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregmagolan Mar 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregmagolan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexeagle Mar 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregmagolan Mar 24, 2020 •

edited

Loading

alexeagle Mar 24, 2020 •

edited

Loading