Skip to content
This repository has been archived by the owner on Sep 16, 2021. It is now read-only.

Get JSCompiler to compile some TS code #74

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all 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
14 changes: 12 additions & 2 deletions WORKSPACE
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@ workspace(name = "build_bazel_rules_typescript")

git_repository(
name = "build_bazel_rules_nodejs",
commit = "1a3a0dea51c0e53a2871fbfaba3be178a308a7a3",
remote = "https://github.com/bazelbuild/rules_nodejs",
tag = "0.2.2",
)

load("@build_bazel_rules_nodejs//:defs.bzl", "node_repositories")
Expand All @@ -33,12 +33,22 @@ node_repositories(package_json = ["//:package.json"])

http_archive(
name = "io_bazel_rules_go",
url = "https://github.com/bazelbuild/rules_go/releases/download/0.7.1/rules_go-0.7.1.tar.gz",
sha256 = "341d5eacef704415386974bc82a1783a8b7ffbff2ab6ba02375e1ca20d9b031c",
url = "https://github.com/bazelbuild/rules_go/releases/download/0.7.1/rules_go-0.7.1.tar.gz",
)

load("@io_bazel_rules_go//go:def.bzl", "go_rules_dependencies", "go_register_toolchains")

go_rules_dependencies()

go_register_toolchains()

git_repository(
name = "io_bazel_rules_closure",
commit = "172f84fe96e07214fa7337b081648d4a61b45b93",
remote = "https://github.com/bazelbuild/rules_closure",
)

load("@io_bazel_rules_closure//closure:defs.bzl", "closure_repositories")

closure_repositories()
40 changes: 40 additions & 0 deletions examples/closure_compiled/BUILD.bazel
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
# Copyright 2017 The Bazel Authors. All rights reserved.
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

package(default_visibility = ["//visibility:public"])

load("@io_bazel_rules_closure//closure:defs.bzl", "closure_js_binary")
load("//:defs.bzl", "ts_library")

ts_library(
name = "child",
srcs = [
"greeter.ts",
"declarations.d.ts",
],
)

ts_library(
name = "parent",
srcs = ["main.ts"],
deps = [":child"],
)

closure_js_binary(
name = "closure_compiled",
deps = [":parent"],
defs = [
"--jscomp_off=reportUnknownTypes",
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should preemptively change this to --jscomp_off=checkTypes . I think that flag disables all the flags that are going to bite us in the future as we expand this example more.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need that? tsickle should suppress checkTypes on each JS file:
https://github.com/angular/tsickle/blob/8ebfd011e9386997ca94001210596c5fd75cd17d/test_files/default/default.js#L3

How do we deal with defs in general? I think internally at Google we've been learning towards wrapping with a macro like ts_binary to make things more uniform. But in open-source there is no uniformity.

/cc @evmar @rkirov

Copy link
Member Author

Choose a reason for hiding this comment

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

We may need both. The generated output definitely has suppression on checkTypes, but without this it is failing to compile ATM

],
)
3 changes: 3 additions & 0 deletions examples/closure_compiled/declarations.d.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
declare function someDeclaredFunction(s: string): string;
Copy link
Contributor

Choose a reason for hiding this comment

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

is there something in a test that asserts this isn't renamed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Right now the example leaves the compiler in simple optimizations which shouldn't suffer from a renaming problem. Given that turning it on will require significant knowledge of JSCompiler I'd like to burn that bridge when we get there since it will involve using the compiled output in a end to end test to verify renaming.


declare const someDeclaredString: string;
6 changes: 6 additions & 0 deletions examples/closure_compiled/greeter.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
export class Greeter {
private greeting = 'hello, world' + someDeclaredFunction(someDeclaredString);
greet() {
return this.greeting;
}
}
3 changes: 3 additions & 0 deletions examples/closure_compiled/main.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
import {Greeter} from './greeter';

console.log(new Greeter().greet());
49 changes: 22 additions & 27 deletions internal/build_defs.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

"""TypeScript rules.
"""

# pylint: disable=unused-argument
# pylint: disable=missing-docstring
load(":common/compilation.bzl", "COMMON_ATTRIBUTES", "compile_ts", "ts_providers_dict_to_struct")
Expand All @@ -25,17 +26,11 @@ def _compile_action(ctx, inputs, outputs, config_file_path):
externs_files = []
non_externs_files = []
for output in outputs:
if output.basename.endswith(".externs.js"):
externs_files.append(output)
elif output.basename.endswith(".es5.MF"):
if output.basename.endswith(".es5.MF"):
ctx.file_action(output, content="")
else:
non_externs_files.append(output)

# TODO(plf): For now we mock creation of files other than {name}.js.
for externs_file in externs_files:
ctx.file_action(output=externs_file, content="")

action_inputs = inputs + [f for f in ctx.files.node_modules
if f.path.endswith(".ts") or f.path.endswith(".json")]
if ctx.file.tsconfig:
Expand Down Expand Up @@ -65,7 +60,6 @@ def _compile_action(ctx, inputs, outputs, config_file_path):
},
)


def _devmode_compile_action(ctx, inputs, outputs, config_file_path):
_compile_action(ctx, inputs, outputs, config_file_path)

Expand Down Expand Up @@ -114,7 +108,6 @@ def tsc_wrapped_tsconfig(ctx,
# ts_library #
# ************ #


def _ts_library_impl(ctx):
"""Implementation of ts_library.

Expand All @@ -132,33 +125,35 @@ def _ts_library_impl(ctx):
ts_library = rule(
_ts_library_impl,
attrs = dict(COMMON_ATTRIBUTES, **{
"srcs":
attr.label_list(
allow_files=FileType([
".ts",
".tsx",
]),
mandatory=True,),
"srcs": attr.label_list(
allow_files = FileType([
".ts",
".tsx",
]),
mandatory = True,
),

# TODO(alexeagle): reconcile with google3: ts_library rules should
# be portable across internal/external, so we need this attribute
# internally as well.
"tsconfig":
attr.label(allow_files = True, single_file = True),
"compiler":
attr.label(
default=get_tsc(),
single_file=False,
allow_files=True,
executable=True,
cfg="host"),
"tsconfig": attr.label(
allow_files = True,
Copy link
Contributor

Choose a reason for hiding this comment

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

curious, did you run buildifier? we should make that part of the process (maybe in a git pre-commit hook?)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I use the same .vimrc at home and at work and you can get buildifier with a relatively simple go install command. I don't even notice it any more

single_file = True,
),
"compiler": attr.label(
default = get_tsc(),
single_file = False,
allow_files = True,
executable = True,
cfg = "host",
),
"supports_workers": attr.bool(default = True),
# @// is special syntax for the "main" repository
# The default assumes the user specified a target "node_modules" in their
# root BUILD file.
"node_modules": attr.label(default = Label("@//:node_modules")),
}),
outputs = {
"tsconfig": "%{name}_tsconfig.json"
}
"tsconfig": "%{name}_tsconfig.json",
},
)
31 changes: 27 additions & 4 deletions internal/common/compilation.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

load(":common/module_mappings.bzl", "module_mappings_aspect")
load(":common/json_marshal.bzl", "json_marshal")
load("@io_bazel_rules_closure//closure/private:defs.bzl", "collect_js")
Copy link
Contributor

Choose a reason for hiding this comment

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

does this require every user of rules_typescript to add rules_closure to their WORKSPACE? I don't think that's desirable, as many users will choose rollup/uglify instead. How can we avoid that?

Copy link
Member Author

Choose a reason for hiding this comment

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

The only way I know to handle this is to copy/paste the code into this repo. You and I both gave the reviewer feedback to Dan asking him to just import it so there is a reflexive repulsion to both strategies.

@mrmeku you had a short implementation of this (<20 lines) could you paste it in here so I can drop it in the Change?


BASE_ATTRIBUTES = dict()

Expand All @@ -33,7 +34,7 @@ COMMON_ATTRIBUTES = dict(BASE_ATTRIBUTES, **{
cfg = "data",
),
# TODO(evanm): make this the default and remove the option.
"runtime": attr.string(default="browser"),
"runtime": attr.string(default = "browser"),
# Used to determine module mappings
"module_name": attr.string(),
"module_root": attr.string(),
Expand All @@ -53,12 +54,11 @@ COMMON_ATTRIBUTES = dict(BASE_ATTRIBUTES, **{
# "diagnostic:regexp", e.g. "TS1234:failed to quizzle the .* wobble".
# Useful to test for expected compilation errors.
"expected_diagnostics": attr.string_list(),

})

COMMON_OUTPUTS = {
# Allow the tsconfig.json to be generated without running compile actions.
"tsconfig": "%{name}_tsconfig.json"
"tsconfig": "%{name}_tsconfig.json",
}

# TODO(plf): Enforce this at analysis time.
Expand Down Expand Up @@ -123,7 +123,6 @@ def _outputs(ctx, label):
declarations = declaration_files,
)


def compile_ts(ctx,
is_library,
extra_dts_files=[],
Expand Down Expand Up @@ -292,6 +291,22 @@ def compile_ts(ctx,
if not is_library:
files += depset(tsickle_externs)

rerooted_es6_sources = []
for source in es6_sources:
root = source.dirname

file_path = "%s/%s" % (root, source.basename.replace(".closure", ""))
rooted_file = ctx.actions.declare_file(file_path)

ctx.actions.expand_template(
output = rooted_file,
template = source,
substitutions = {}
)
rerooted_es6_sources.append(rooted_file)

js = collect_js(ctx, ctx.attr.deps)

return {
"files": files,
"runfiles": ctx.runfiles(
Expand All @@ -301,6 +316,14 @@ def compile_ts(ctx,
collect_default=True,
collect_data=True,
),
# All of the subproviders below are considered optional and MUST be
# accessed using getattr(x, y, default). See collect_js() in
# @io_bazel_rules_closure//closure/private/defs.bzl.
"closure_js_library": struct(
# NestedSet<File> of all JavaScript source File artifacts in the
# transitive closure. These files MUST be JavaScript.
srcs= js.srcs + rerooted_es6_sources,
),
# TODO(martinprobst): Prune transitive deps, only re-export what's needed.
"typescript": {
"declarations": declarations,
Expand Down
20 changes: 20 additions & 0 deletions internal/tsc_wrapped/tsc_wrapped.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import * as fs from 'fs';
import * as path from 'path';
import * as tsickle from 'tsickle';
import * as ts from 'typescript';

import {PLUGIN as tsetsePlugin} from '../tsetse/runner';
Expand Down Expand Up @@ -128,6 +129,25 @@ function runOneBuild(
return false;
}

const emitResults: tsickle.EmitResult[] =
program.getSourceFiles()
.filter(isCompilationTarget)
.map(
file => tsickle.emitWithTsickle(
program, compilerHost, (compilerHost as ts.CompilerHost),
options, file));

const emitResult = tsickle.mergeEmitResults(emitResults);

let externs = '/** @externs */';
if (bazelOpts.tsickleGenerateExterns) {
externs += tsickle.getGeneratedExterns(emitResult.externs);
}

if (bazelOpts.tsickleExternsPath) {
fs.writeFileSync(bazelOpts.tsickleExternsPath, externs);
}

for (const sf of program.getSourceFiles().filter(isCompilationTarget)) {
const emitResult = program.emit(
sf, /*writeFile*/ undefined,
Expand Down