Get JSCompiler to compile some TS code #74

achew22 · 2017-11-19T06:31:00Z

Months of teath nashing later...

Very far from perfect, but it produces a bundled JS. Launch and iterate... right?

Fixes #6

codesuki · 2017-11-20T01:05:48Z

internal/common/compilation.bzl

+        # Bazel < 0.8.0.
+        "bazel-out/local-fastbuild/bin/",
+        # Bazel >= 0.8.0.
+        "bazel-out/k8-fastbuild/bin/",


On OSX it seems to be darwin_x86_64-fastbuild. Maybe you could add this.

mrmeku

Just some comments of how to address a todo and how to clean up the code a bit

mrmeku · 2017-11-20T22:03:47Z

examples/closure_compiled/BUILD.bazel

+        "--dependency_mode=LOOSE",
+        "--js_module_root=bazel-out/k8-fastbuild/bin/examples/closure_compiled/examples/closure_compiled",
+        "--jscomp_off=reportUnknownTypes",
+        "--jscomp_off=missingSourcesWarnings",


This should be the only def you need to keep. Remove the rest

mrmeku · 2017-11-20T22:04:23Z

internal/common/compilation.bzl

@@ -301,6 +327,49 @@ def compile_ts(ctx,
          collect_default=True,
          collect_data=True,
      ),
+      "closure_js_library": struct(
+          # File pointing to a ClosureJsLibrary protobuf file in pbtxt format
+          # that's generated by this specific Target. It contains some metadata


Remove the unused comments?

mrmeku · 2017-11-20T22:04:35Z

internal/common/compilation.bzl

+          # within any given transitive closure, no namespace collisions
+          # exist. These MUST NOT begin with "/" or ".", or contain "..".
+          ###modules=js.modules + modules,
+          # NestedSet<File> of all protobuf definitions in the transitive


Remove the unused comments?

mrmeku · 2017-11-20T22:07:03Z

internal/common/compilation.bzl

@@ -292,6 +290,34 @@ def compile_ts(ctx,
  if not is_library:
    files += depset(tsickle_externs)

+  rerooted_es6_sources = []
+  es6_js_modules_roots = [] # todo(achew22): this is non recursive... recurse.


Use this code as a base to make this recursive: https://github.com/bazelbuild/rules_closure/blob/master/closure/private/defs.bzl#L61

googlebot · 2017-11-23T04:56:04Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this State. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

achew22 · 2017-11-28T20:13:25Z

Hey @alexeagle. I think this works as is. Would you mind taking a look at it?

@mrmeku can you explicitly state you'd be okay with this PR being merged?

mrmeku

Aside from where I commented I think this is ready for merging.

mrmeku · 2017-11-28T20:46:53Z

WORKSPACE

    sha256 = "91fca9cf860a1476abdc185a5f675b641b60d3acf0596679a27b580af60bf19c",
+    url = "https://github.com/bazelbuild/rules_go/releases/download/0.7.0/rules_go-0.7.0.tar.gz",


nit: Consider reverting this part

FYI I have a PR out that upgrades this to 0.7.1

mrmeku · 2017-11-28T20:49:13Z

examples/closure_compiled/BUILD.bazel

+    name = "closure_compiled",
+    deps = [":parent"],
+    defs = [
+        "--jscomp_off=reportUnknownTypes",


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.

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

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

mrmeku · 2017-11-28T20:54:24Z

internal/common/compilation.bzl

@@ -292,6 +291,25 @@ def compile_ts(ctx,
  if not is_library:
    files += depset(tsickle_externs)

+  rerooted_es6_sources = []
+  es6_js_modules_roots = []


You can remove the entire part about es6_js_module_roots and this still compiles. I think you should remove it for now because we obviously don't quite understand what this does correctly. As far as what I've read from Jart commenting on github, these roots are just stripped off from es6 import statements. I think they might only be applicable for absolute import statements (since Bazel creates a new file system)

yohcop · 2017-11-28T21:34:19Z

WORKSPACE

-    remote = "https://github.com/bazelbuild/rules_nodejs",
-    tag = "0.1.11",
+    commit = "f6eb823e1140da6eafbf77f56a249f72bb89416e",
+    remote = "https://github.com/achew22/rules_nodejs",


Is this right? Should it be reverted before merging?

alexeagle

awesome work pushing this forward

alexeagle · 2017-11-28T21:26:34Z

WORKSPACE

-    remote = "https://github.com/bazelbuild/rules_nodejs",
-    tag = "0.1.11",
+    commit = "f6eb823e1140da6eafbf77f56a249f72bb89416e",
+    remote = "https://github.com/achew22/rules_nodejs",


Do you have a PR on rules_nodejs that has the bits you need? We can't have rules_typescript on master pointing to your fork of a dependency

It doesn't actually matter now. That change was to allow 0.8.0rcN work when I was testing. now that 0.8.0 is out it shouldn't matter.

alexeagle · 2017-11-28T21:26:54Z

WORKSPACE

    sha256 = "91fca9cf860a1476abdc185a5f675b641b60d3acf0596679a27b580af60bf19c",
+    url = "https://github.com/bazelbuild/rules_go/releases/download/0.7.0/rules_go-0.7.0.tar.gz",


FYI I have a PR out that upgrades this to 0.7.1

alexeagle · 2017-11-28T21:28:27Z

examples/closure_compiled/BUILD.bazel

+    name = "closure_compiled",
+    deps = [":parent"],
+    defs = [
+        "--jscomp_off=reportUnknownTypes",


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

alexeagle · 2017-11-28T21:29:31Z

examples/closure_compiled/declarations.d.ts

@@ -0,0 +1,3 @@
+declare function someDeclaredFunction(s: string): string;


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

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.

alexeagle · 2017-11-28T21:30:06Z

internal/build_defs.bzl

-                executable=True,
-                cfg="host"),
+        "tsconfig": attr.label(
+            allow_files = True,


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

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

alexeagle · 2017-11-28T21:37:25Z

internal/common/compilation.bzl

@@ -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")


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?

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?

alexeagle · 2017-11-28T21:39:12Z

internal/common/compilation.bzl

+    root = source.dirname
+    es6_js_modules_roots.append(root)
+
+    file_path = "%s/%s" % (root, source.basename.replace(".closure", ""))


do we still produce the foo.closure.js outputs too?
I expected to see a change around https://github.com/bazelbuild/rules_typescript/blob/master/examples/es6_output/es6_output_test.sh#L5

This copies the file to a new nested location so it shouldn't remove them. That is to say this test passes.

This looks to me like a fairly brittle test. Seems to me adding another one for this process is unnecessary and would be better verified by adding a closure_js_binary rule that actually consumes the output and emits a compiled .js (then possibly runs it through e2e with a browser).

Months of teath nashing later...

Every the ts_library impl now returns a struct named "closure_js_library" needed by both closure_js_library and closure_js_binary. This struct contains the srcs of the ts_library transpiled to es6 as well as the path prefixes which should be stripped off of import statements in order to find the imported file within the file system.

mrmeku · 2017-12-20T23:16:34Z

You can close this now Andrew, I've done all this logic in a more Alex approved way in #112

achew22 · 2017-12-20T23:17:40Z

Perfect!

googlebot added the cla: yes label Nov 19, 2017

achew22 force-pushed the closure_compiler branch 2 times, most recently from 26e6e62 to d900a87 Compare November 19, 2017 22:23

codesuki reviewed Nov 20, 2017

View reviewed changes

mrmeku suggested changes Nov 20, 2017

View reviewed changes

mrmeku mentioned this pull request Nov 22, 2017

Interop ts_library with closure_js_binary. #76

Closed

achew22 force-pushed the closure_compiler branch from d900a87 to 018a0b7 Compare November 23, 2017 04:56

googlebot added cla: no and removed cla: yes labels Nov 23, 2017

pshields mentioned this pull request Nov 28, 2017

bazel build @build_bazel_rules_typescript//... fails from other workspaces #85

Closed

mrmeku reviewed Nov 28, 2017

View reviewed changes

yohcop reviewed Nov 28, 2017

View reviewed changes

alexeagle reviewed Nov 28, 2017

View reviewed changes

achew22 and others added 3 commits November 30, 2017 23:00

Get JSCompiler to compile some TS code

7716fe9

Months of teath nashing later...

Extract extern generation from Google3

4569ae5

achew22 force-pushed the closure_compiler branch from 018a0b7 to 4569ae5 Compare December 1, 2017 06:03

alexeagle mentioned this pull request Dec 5, 2017

Example of using closure_js_binary to bundle an Angular application? angular/angular-bazel-example#15

Closed

achew22 closed this Dec 20, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Get JSCompiler to compile some TS code #74

Get JSCompiler to compile some TS code #74

achew22 commented Nov 19, 2017

codesuki Nov 20, 2017

mrmeku left a comment

mrmeku Nov 20, 2017

mrmeku Nov 20, 2017

mrmeku Nov 20, 2017

mrmeku Nov 20, 2017

googlebot commented Nov 23, 2017

achew22 commented Nov 28, 2017

mrmeku left a comment

mrmeku Nov 28, 2017

alexeagle Nov 28, 2017

achew22 Nov 30, 2017

mrmeku Nov 28, 2017

alexeagle Nov 28, 2017

achew22 Dec 1, 2017

mrmeku Nov 28, 2017

yohcop Nov 28, 2017

alexeagle left a comment

alexeagle Nov 28, 2017

achew22 Nov 30, 2017

alexeagle Nov 28, 2017

alexeagle Nov 28, 2017

alexeagle Nov 28, 2017

achew22 Nov 30, 2017

alexeagle Nov 28, 2017

achew22 Nov 30, 2017

alexeagle Nov 28, 2017

achew22 Nov 30, 2017

alexeagle Nov 28, 2017

achew22 Nov 30, 2017

mrmeku commented Dec 20, 2017

achew22 commented Dec 20, 2017

		sha256 = "91fca9cf860a1476abdc185a5f675b641b60d3acf0596679a27b580af60bf19c",
		url = "https://github.com/bazelbuild/rules_go/releases/download/0.7.0/rules_go-0.7.0.tar.gz",

		@@ -0,0 +1,3 @@
		declare function someDeclaredFunction(s: string): string;

Get JSCompiler to compile some TS code #74

Get JSCompiler to compile some TS code #74

Conversation

achew22 commented Nov 19, 2017

Choose a reason for hiding this comment

mrmeku 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

googlebot commented Nov 23, 2017

achew22 commented Nov 28, 2017

mrmeku 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

Choose a reason for hiding this comment

alexeagle 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

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

mrmeku commented Dec 20, 2017

achew22 commented Dec 20, 2017