-
Notifications
You must be signed in to change notification settings - Fork 94
Get JSCompiler to compile some TS code #74
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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", | ||
], | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
declare function someDeclaredFunction(s: string): string; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is there something in a test that asserts this isn't renamed? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; |
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; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
import {Greeter} from './greeter'; | ||
|
||
console.log(new Greeter().greet()); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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") | ||
|
@@ -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: | ||
|
@@ -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) | ||
|
||
|
@@ -114,7 +108,6 @@ def tsc_wrapped_tsconfig(ctx, | |
# ts_library # | ||
# ************ # | ||
|
||
|
||
def _ts_library_impl(ctx): | ||
"""Implementation of ts_library. | ||
|
||
|
@@ -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, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
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", | ||
}, | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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() | ||
|
||
|
@@ -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(), | ||
|
@@ -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. | ||
|
@@ -123,7 +123,6 @@ def _outputs(ctx, label): | |
declarations = declaration_files, | ||
) | ||
|
||
|
||
def compile_ts(ctx, | ||
is_library, | ||
extra_dts_files=[], | ||
|
@@ -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( | ||
|
@@ -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, | ||
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 likets_binary
to make things more uniform. But in open-source there is no uniformity./cc @evmar @rkirov
There was a problem hiding this comment.
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