Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(typescript): add ts_project rule #1710

Merged
merged 6 commits into from
Mar 24, 2020
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions examples/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,9 @@ example_integration_test(
name = "examples_react_webpack",
# TODO: add some tests in the example
bazel_commands = ["build ..."],
npm_packages = {
"//packages/typescript:npm_package": "@bazel/typescript",
},
# TODO(alexeagle): somehow this is broken by the new node-patches based node_patches script
# ERROR: D:/temp/tmp-6900sejcsrcttpdb/BUILD.bazel:37:1: output 'app.bundle.js' was not created
tags = ["no-bazelci-windows"],
Expand Down
32 changes: 17 additions & 15 deletions examples/react_webpack/BUILD.bazel
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
load("@npm//http-server:index.bzl", "http_server")
load("@npm//sass:index.bzl", "sass")
load("@npm//typescript:index.bzl", "tsc")
load("@npm//webpack-cli:index.bzl", webpack = "webpack_cli")
load("@npm_bazel_typescript//:index.bzl", "ts_project")

sass(
name = "styles",
Expand All @@ -13,27 +13,29 @@ sass(
data = ["styles.scss"],
)

tsc(
name = "compile",
outs = ["index.js"],
args = [
"$(execpath index.tsx)",
"$(execpath types.d.ts)",
"--outDir",
"$(RULEDIR)",
"--lib",
"es2015,dom",
"--jsx",
"react",
],
data = [
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,
alexeagle marked this conversation as resolved.
Show resolved Hide resolved
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",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

actual = "tsconfig",
)

webpack(
name = "bundle",
outs = ["app.bundle.js"],
Expand Down
4 changes: 4 additions & 0 deletions examples/react_webpack/WORKSPACE
Original file line number Diff line number Diff line change
Expand Up @@ -19,3 +19,7 @@ yarn_install(
package_json = "//:package.json",
yarn_lock = "//:yarn.lock",
)

load("@npm//:install_bazel_dependencies.bzl", "install_bazel_dependencies")

install_bazel_dependencies()
1 change: 1 addition & 0 deletions examples/react_webpack/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
"@bazel/bazelisk": "^1.3.0",
"@bazel/buildifier": "^0.29.0",
"@bazel/ibazel": "^0.12.2",
"@bazel/typescript": "^1.4.1",
"@types/react": "^16.9.5",
"@types/react-dom": "^16.9.1",
"css-loader": "^3.2.0",
Expand Down
6 changes: 6 additions & 0 deletions examples/react_webpack/tsconfig.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
{
"compilerOptions": {
"jsx": "react",
"lib": ["ES2015", "DOM"]
}
}
2 changes: 2 additions & 0 deletions packages/typescript/src/index.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,13 @@ Users should not load files under "/internal"

load("//internal:build_defs.bzl", _ts_library = "ts_library_macro")
load("//internal:ts_config.bzl", _ts_config = "ts_config")
load("//internal:ts_project.bzl", _ts_project = "ts_project_macro")
load("//internal:ts_repositories.bzl", _ts_setup_workspace = "ts_setup_workspace")
load("//internal/devserver:ts_devserver.bzl", _ts_devserver = "ts_devserver_macro")

ts_setup_workspace = _ts_setup_workspace
ts_library = _ts_library
ts_config = _ts_config
ts_devserver = _ts_devserver
ts_project = _ts_project
# If adding rules here also add to index.docs.bzl
2 changes: 2 additions & 0 deletions packages/typescript/src/index.docs.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,14 @@ So this is a copy of index.bzl with macro indirection removed.

load("//internal:build_defs.bzl", _ts_library = "ts_library")
load("//internal:ts_config.bzl", _ts_config = "ts_config")
load("//internal:ts_project.bzl", _ts_project = "ts_project_macro")
load("//internal:ts_repositories.bzl", _ts_setup_workspace = "ts_setup_workspace")
load("//internal/devserver:ts_devserver.bzl", _ts_devserver = "ts_devserver")

ts_setup_workspace = _ts_setup_workspace
ts_library = _ts_library
ts_config = _ts_config
ts_project = _ts_project
ts_devserver = _ts_devserver
# DO NOT ADD MORE rules here unless they appear in the generated docsite.
# Run yarn stardoc to re-generate the docsite.
1 change: 1 addition & 0 deletions packages/typescript/src/internal/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ filegroup(
srcs = [
"build_defs.bzl",
"ts_config.bzl",
"ts_project.bzl",
"ts_repositories.bzl",
"//internal/devserver:package_contents",
],
Expand Down
273 changes: 273 additions & 0 deletions packages/typescript/src/internal/ts_project.bzl
Original file line number Diff line number Diff line change
@@ -0,0 +1,273 @@
"ts_project rule"

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

_ATTRS = {
Copy link
Collaborator

@gregmagolan gregmagolan Mar 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. 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
  2. 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`

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added args pass-through with a delightful test coverage

# 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),
alexeagle marked this conversation as resolved.
Show resolved Hide resolved
"extends": attr.label_list(allow_files = [".json"]),
"tsc": attr.label(default = Label("@npm//typescript/bin:tsc"), executable = True, cfg = "host"),

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"tsconfig": attr.label(mandatory = True, allow_single_file = [".json"]),
"deps": attr.label_list(providers = [DeclarationInfo]),
}

# tsc knows how to produce the following kinds of output files.
# NB: the macro `ts_project_macro` will set these outputs based on user
# telling us which settings are enabled in the tsconfig for this project.
_OUTPUTS = {
"buildinfo_out": attr.output(),
"js_outs": attr.output_list(),
alexeagle marked this conversation as resolved.
Show resolved Hide resolved
"map_outs": attr.output_list(),
"typing_maps_outs": attr.output_list(),
"typings_outs": attr.output_list(),
}

_TsConfigInfo = provider(
doc = """Passes tsconfig.json files to downstream compilations so that TypeScript can read them.
This is needed to support Project References""",
fields = {
"tsconfigs": "depset of tsconfig.json files",
},
)

def _ts_project_impl(ctx):
arguments = ctx.actions.args()
arguments.add_all([
"-p",
ctx.file.tsconfig.short_path,
"--outDir",
"/".join([ctx.bin_dir.path, ctx.label.package]),
])
if len(ctx.outputs.typings_outs) > 0:
arguments.add_all([
"--declarationDir",
"/".join([ctx.bin_dir.path, ctx.label.package]),
])

# When users report problems, we can ask them to re-build with
# --define=VERBOSE_LOGS=1
# so anything that's useful to diagnose rule failures belongs here
if "VERBOSE_LOGS" in ctx.var.keys():
arguments.add_all([
# What files were in the ts.Program
"--listFiles",
# Did tsc write all outputs to the place we expect to find them?
"--listEmittedFiles",
# Why did module resolution fail?
"--traceResolution",
# Why was the build slow?
"--diagnostics",
"--extendedDiagnostics",
])

deps_depsets = []
for dep in ctx.attr.deps:
if _TsConfigInfo in dep:
deps_depsets.append(dep[_TsConfigInfo].tsconfigs)
if NpmPackageInfo in dep:
# 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:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

deps_depsets.append(dep[DeclarationInfo].transitive_declarations)

inputs = ctx.files.srcs + depset(transitive = deps_depsets).to_list() + [ctx.file.tsconfig]
if ctx.attr.extends:
inputs.extend(ctx.files.extends)
outputs = ctx.outputs.js_outs + ctx.outputs.map_outs + ctx.outputs.typings_outs + ctx.outputs.typing_maps_outs
if ctx.outputs.buildinfo_out:
outputs.append(ctx.outputs.buildinfo_out)

if len(outputs) == 0:
return []
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should still return transitive declarations & tsconfigs?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


run_node(
ctx,
inputs = inputs,
arguments = [arguments],
outputs = outputs,
executable = "tsc",
progress_message = "Compiling TypeScript project %s" % ctx.file.tsconfig.short_path,
)

runtime_files = depset(ctx.outputs.js_outs + ctx.outputs.map_outs)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

typings_files = ctx.outputs.typings_outs + [s for s in ctx.files.srcs if s.path.endswith(".d.ts")]

return [
DeclarationInfo(
declarations = depset(typings_files),
transitive_declarations = depset(typings_files, transitive = [
dep[DeclarationInfo].transitive_declarations
for dep in ctx.attr.deps
]),
),
DefaultInfo(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

@alexeagle alexeagle Mar 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

files = runtime_files,
runfiles = ctx.runfiles(
transitive_files = runtime_files,
collect_default = True,
),
),
_TsConfigInfo(tsconfigs = depset([ctx.file.tsconfig], transitive = [
dep[_TsConfigInfo].tsconfigs
for dep in ctx.attr.deps
if _TsConfigInfo in dep
])),
]

ts_project = rule(
implementation = _ts_project_impl,
attrs = dict(_ATTRS, **_OUTPUTS),
)

def _out_paths(srcs, ext):
return [f[:f.rindex(".")] + ext for f in srcs if not f.endswith(".d.ts")]

def ts_project_macro(
name = "tsconfig",
tsconfig = None,
srcs = None,
deps = [],
extends = None,
declaration = False,
source_map = False,
declaration_map = False,
composite = False,
incremental = False,
emit_declaration_only = False,
Toxicable marked this conversation as resolved.
Show resolved Hide resolved
tsc = "@npm//typescript/bin:tsc",
**kwargs):
"""Compiles one TypeScript project using `tsc -p`

Unlike `ts_library`, this rule is the thinnest possible layer of Bazel interoperability on top
of the TypeScript compiler. It shifts the burden of configuring TypeScript into the tsconfig.json file.
TODO(alexeagle): update https://github.com/bazelbuild/rules_nodejs/blob/master/docs/TypeScript.md#alternatives
to describe the trade-offs between the two rules.

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
`bazel-out/[target architecture]/bin/path/to/package`
so we override whatever settings appear in your tsconfig.
- Bazel expects that each output is produced by a single rule.
Thus if you have two `ts_project` rules with overlapping sources (the same `.ts` file
appears in more than one) then you get an error about conflicting `.js` output
files if you try to build both together.
Worse, if you build them separately then the output directory will contain whichever
one you happened to build most recently. This is highly discouraged.

> Note: in order for TypeScript to resolve relative references to the bazel-out folder,
> we recommend that the base tsconfig contain a rootDirs section that includes all
> possible locations they may appear.
>
> We hope this will not be needed in some future release of TypeScript.
> Follow https://github.com/microsoft/TypeScript/issues/37257 for more info.
>
> For example, if the base tsconfig file relative to the workspace root is
> `path/to/tsconfig.json` then you should configure like:
>
> ```
> "compilerOptions": {
> "rootDirs": [
> ".",
> "../../bazel-out/darwin-fastbuild/bin/path/to",
> "../../bazel-out/k8-fastbuild/bin/path/to",
> "../../bazel-out/x64_windows-fastbuild/bin/path/to",
> "../../bazel-out/darwin-dbg/bin/path/to",
> "../../bazel-out/k8-dbg/bin/path/to",
> "../../bazel-out/x64_windows-dbg/bin/path/to",
> ]
> }
> ```

> 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

Args:
name: A name for the target.

We recommend you use the basename (no `.json` extension) of the tsconfig file that should be compiled.

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

deps: List of labels of other rules that produce TypeScript typings (.d.ts files)

tsconfig: Label of the tsconfig.json file to use for the compilation.

By default, we add `.json` to the `name` attribute.

extends: List of labels of tsconfig file(s) referenced in `extends` section of tsconfig.

Must include any tsconfig files "chained" by extends clauses.

tsc: Label of the TypeScript compiler binary to run.

Override this if your npm_install or yarn_install isn't named "npm"
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.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added above

Instructs Bazel to expect a `.d.ts` output for each `.ts` source.
source_map: if the `sourceMap` bit is set in the tsconfig.
Instructs Bazel to expect a `.js.map` output for each `.ts` source.
declaration_map: if the `declarationMap` bit is set in the tsconfig.
Instructs Bazel to expect a `.d.ts.map` output for each `.ts` source.
composite: if the `composite` bit is set in the tsconfig.
Instructs Bazel to expect a `.tsbuildinfo` output and a `.d.ts` output for each `.ts` source.
incremental: if the `incremental` bit is set in the tsconfig.
Instructs Bazel to expect a `.tsbuildinfo` output.
emit_declaration_only: if the `emitDeclarationOnly` bit is set in the tsconfig.
Instructs Bazel *not* to expect `.js` or `.js.map` outputs for `.ts` sources.
"""

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

if tsconfig == None:
tsconfig = name + ".json"

ts_project(
name = name,
srcs = srcs,
deps = deps,
tsconfig = tsconfig,
extends = extends,
js_outs = _out_paths(srcs, ".js") if not emit_declaration_only else [],
map_outs = _out_paths(srcs, ".js.map") if source_map and not emit_declaration_only else [],
typings_outs = _out_paths(srcs, ".d.ts") if declaration or composite else [],
typing_maps_outs = _out_paths(srcs, ".d.ts.map") if declaration_map else [],
buildinfo_out = tsconfig[:-5] + ".tsbuildinfo" if composite or incremental else None,
tsc = tsc,
**kwargs
)
1 change: 1 addition & 0 deletions packages/typescript/test/ts_project/BUILD
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
exports_files(["tsconfig-base.json"])
Loading