Added closure_ts_binary rules to interop ts_library with closure. #112

mrmeku · 2017-12-20T22:30:52Z

Added a hello world AngularJS example to show how to use the rule.

pshields

Hi @mrmeku, I'd like to join the review for this, if that's okay. I've been following this PR with interest. I'm still learning how Skylark and the Closure rules work, so please forgive me if my questions are pretty basic.

pshields · 2018-01-02T17:48:49Z

internal/build_defs.bzl

        "--js=node_modules/hammerjs/hammer.js",
        "--js=%s" % rxjs_path,

        ### @angular packages
        "--package_json_entry_names=es2015",
-        # Core must be specified seperately and not globbed with packages which depend on it 
+        "--js=node_modules/@angular/**/package.json",
+        # Core must be specified individually and not globbed with packages which depend on it 


Is this comment still accurate? It looks like this commit removes the individual specification of @angular/core/package.json.

pshields · 2018-01-02T18:22:59Z

internal/build_defs.bzl

+        ### All other filegroup, Closure Library, and rerooted Typescript files.
+        "--js={0}/**.js,!{1},!{0}/**.ngsummary.js".format(rerooted_node_modules_path, rxjs_path),
+    ],
+    manually_specify_js = True,


This line causes errors for me if I try to use closure_ng_binary. It looks like this attribute gets forwarded to closure_js_binary which doesn't recognize it.

mrmeku · 2018-01-02T19:16:07Z

@pshields The more the merrier. I'm happy to answer anything that is unclear for you.

…

On Tue, Jan 2, 2018, 11:26 AM Patrick Shields ***@***.***> wrote: ***@***.**** commented on this pull request. Hi @mrmeku <https://github.com/mrmeku>, I'd like to join the review for this, if that's okay. I've been following this PR with interest. I'm still learning how Skylark and the Closure rules work, so please forgive me if my questions are pretty basic. ------------------------------ In internal/build_defs.bzl <#112 (comment)> : > "--js=node_modules/hammerjs/hammer.js", "--js=%s" % rxjs_path, ### @angular packages "--package_json_entry_names=es2015", - # Core must be specified seperately and not globbed with packages which depend on it + ***@***.***/**/package.json", + # Core must be specified individually and not globbed with packages which depend on it Is this comment still accurate? It looks like this commit removes the individual specification of @angular/core/package.json. ------------------------------ In internal/build_defs.bzl <#112 (comment)> : > + "--js=node_modules/hammerjs/hammer.js", + "--js=%s" % rxjs_path, + + ### @angular packages + "--package_json_entry_names=es2015", + ***@***.***/**/package.json", + # Core must be specified individually and not globbed with packages which depend on it + # (ie. @angular/animations). + ***@***.***/core/esm2015/core.js", + # Glob the other @angular packages without core. + ***@***.******@***.***/core/esm2015/core.js", + + ### All other filegroup, Closure Library, and rerooted Typescript files. + "--js={0}/**.js,!{1},!{0}/**.ngsummary.js".format(rerooted_node_modules_path, rxjs_path), + ], + manually_specify_js = True, This line causes errors for me if I try to use closure_ng_binary. It looks like this attribute gets forwarded to closure_js_binary which doesn't recognize it. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#112 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHteOzN1vNYrz3en67JEo-VDg5GcYeZLks5tGnTDgaJpZM4RJDJj> .

This function is exposed so that production bundling rules (rollup, closure, unglify etc.) can invoke this function to get a file tree contianing only production files with the .closure.js suffix stripped off. Changed es5_output and es6_output to test the same example to ensure that both es5 and es6 generation work together happily. Added closure_ts_binary rules to interop ts_library with closure. Added a fix for extern generation. Added a hello world AngularJS example to show how to use the rule. Changed npm run test to exercise the production bundling rule. Added support for compiling modules loaded using workspace name First working local demo of ABC with this commit

pshields

I'm getting some Closure compilation errors when I try the latest commit on my project. It looks like a potential issue with the import paths on the rerooted files. Suppose I have all my TypeScript code in a ui/ directory inside my workspace root. The generated file tree lives in, say, bazel-out/k8-fastbuild/bin/ui/<my_closure_ng_binary_target>_collect_es6_sources.es6/node_modules/<my_workspace_name>/ui/. And the generated files in that directory have import statements that look like import * as i1 from "ui/app.module";. I'm getting ERROR - Failed to load module "ui/app.module" Closure compiler errors.

I'm guessing that if the import were relative to the current file (e.g. import * as i1 from "./app.module") or relative to node_modules in the generated file tree (e.g. import * as i1 from "<my_workspace_name>/ui/app.module"), it would work. Instead, since the generated code's import paths seem to be relative to node_modules/<my_workspace_root> instead of node_modules, I'm guessing the Closure compiler isn't able to correctly resolve the imports.

Can we configure Closure to resolve those imports correctly? Or do the import paths themselves need to be corrected?

pshields · 2018-01-03T14:34:11Z

internal/common/collect_es6_sources.bzl

+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+"""Used by production rules to expose a file tree of only prod files.


Maybe say why someone might need a file tree of only prod files?

pshields · 2018-01-03T14:38:08Z

internal/common/collect_es6_sources.bzl

+"""
+
+def collect_es6_sources(ctx):
+  """Returns a file tree containing only production es6 sources.


It's not clear to me whether this is supposed to be pre- or post-Closure compilation. Or does it matter? Consider mentioning where Closure compilation comes into this.

alexeagle · 2018-01-03T22:32:42Z

@pshields the fact that you can import from 'ui/app.module' is a bug, which I should fix immediately since it's breaking and maybe lots of users rely on that.
If you want an absolute import, it should be from my_workspace/ui/app.module.

pshields · 2018-01-04T01:08:45Z

@alexeagle FYI, the workspace-relative imports are coming from the generated app.module.ngfactory.js file, not from the code I control (app.module.ts). I am using the absolute imports from my .ts files.

@mrmeku looks like you removed closure_ng_binary from this PR - will that be in a separate PR?

alexeagle · 2018-01-04T02:04:38Z

The angular codegen bug is angular/angular#21022
@mrmeku and I tracked it down and you can see a commit referenced from that issue that I need to land.

We decided not to have a closure_ng_binary I believe. Need to sync with Dan to reboot my understanding following vacation...

mrmeku · 2018-01-04T21:30:26Z

The need for having a separate closure_ng_binary rules has largely gone away due to me removing the need to pass most previously required flags that need to be passed down to closure_js_binary. My ultimate goal is to make the number of flags necessary to pass down to be 0 for hello world projects (we need to compile angular from source and fix up Tsickle to interact with RXJS before we can get there though).

Here's my PR for the Angular issue you both referenced: angular/angular#21316

alexeagle · 2018-01-05T00:37:57Z

BUILD.bazel

@@ -20,6 +20,8 @@
 # gazelle:exclude node_modules
 load("@io_bazel_rules_go//go:def.bzl", "gazelle")

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


alexeagle · 2018-01-05T02:15:23Z

.circleci/config.yml

@@ -71,7 +71,10 @@ jobs:
          key: *cache_key
      - run: bazel run @yarn//:yarn
      # Don't occupy the bazel server, as this test wants to run Bazel itself


Add a TODO here to replace this shenanigans with a protractor rule when we have one.

Do you think there's enough speed advantage to run the -dev and -prod variants in parallel by making two jobs? probably not...

alexeagle · 2018-01-05T02:15:52Z

BUILD.bazel

@@ -37,3 +39,4 @@ filegroup(
    ]),
    visibility = ["//visibility:public"],
 )
+


can you do a pass through your PRs in general to remove spurious unneeded deltas so the review is easier?

alexeagle · 2018-01-05T02:16:43Z

WORKSPACE

@@ -52,3 +52,13 @@ http_archive(
    url = "https://github.com/bazelbuild/bazel/releases/download/0.9.0/bazel-0.9.0-dist.zip",
    sha256 = "efb28fed4ffcfaee653e0657f6500fc4cbac61e32104f4208da385676e76312a",
 )
+
+git_repository(


add a comment explaining why we depend on this. Does it affect users? Do they need to add this to their WORKSPACE as well?

alexeagle · 2018-01-05T02:17:50Z

examples/app/BUILD.bazel

+# Needed because the devserver only loads static files that appear under this
+# package.
+genrule(
+    name = "angular_js",


Why should the canonical typescript app example use Angular? We should be careful about layering, this rule is generally useful to all typescript users

alexeagle · 2018-01-05T02:18:11Z

examples/app/BUILD.bazel

+    entry_module = "build_bazel_rules_typescript/examples/app/main",
+)
+
+# We do not want closure to compile angular.js, and instead will serve angular.min.js.


I think this angularjs example belongs somewhere else

alexeagle · 2018-01-05T02:25:32Z

internal/build_defs.bzl

+    implementation = _collect_es6_sources_impl,
+)
+
+def closure_ts_binary(name, deps, **kwargs):


I think we discussed that I'd rather let users see the closure_js_binary in its full glory, and direct their questions and wrath to owners of those rules.
I think it might be better for users to just expose the collect_sources as a closure_js_library dropin replacement:

ts_library(name = foo) # This rule converts its deps to act like a `closure_js_library` closure_ts_library(name = "foo_closure", deps = [":foo"]) closure_js_binary(deps = [":foo_closure"])

it means one more rule in users BUILD files but I think it's better to expose the fundamental building blocks rather than introduce a leaky abstraction.

alexeagle · 2018-01-05T02:25:47Z

internal/common/collect_es6_sources.bzl

@@ -0,0 +1,48 @@
+# Copyright 2017 The Bazel Authors. All rights reserved.


this file is part of a PR in rules_nodejs right?

alexeagle · 2018-01-05T02:26:12Z

internal/tsc_wrapped/compiler_host.ts

@@ -131,8 +131,8 @@ export class CompilerHost implements ts.CompilerHost, tsickle.TsickleHost {

  /** Allows suppressing warnings for specific known libraries */
  shouldIgnoreWarningsForPath(filePath: string): boolean {
-    return this.bazelOpts.ignoreWarningPaths.some(
-        p => !!filePath.match(new RegExp(p)));
+    return (this.bazelOpts.ignoreWarningPaths || [])


again, I think we had this change somewhere else?

alexeagle · 2018-01-05T02:26:22Z

internal/tsc_wrapped/package.json

@@ -5,7 +5,7 @@
    "@types/node": "7.0.18",
    "@types/source-map": "0.5.2",
    "protobufjs": "5.0.0",
-    "tsickle": "0.25.5",
+    "tsickle": "0.25.6",


is this required?

alexeagle · 2018-01-05T02:27:02Z

internal/tsc_wrapped/tsc_wrapped.ts

@@ -145,6 +146,25 @@ function runOneBuild(
    return false;
  }

+  const emitResults: tsickle.EmitResult[] =


add a TODO for me to reconcile this with the google3 version.

eventually there should be only one tsc_wrapped.ts

alexeagle · 2018-01-12T18:52:00Z

Note, fixes #6

bazelbuild/rules_typescript#112 is an in-progress PR that will add a closure_ts_library or closure_ts_binary rule to rules_typescript. but the review hasn't progressed for a month. in Ubehebe/rules_typescript@dd4169d, i patched in the PR and got it to work. it's not pretty, but it allows per-target conversion of closure_js_library to closure_ts_library (and eventually just ts_library). closure_ts_library transpiles ts to es6. the main question for this codebase is how es6 modules interoperate with goog.modules. jscompiler has some amount of interop support: see https://github.com/google/closure-compiler/wiki/JS-Modules and google/closure-compiler#2644. in particular, es6 modules (`import`) can be required from goog.modules (`goog.require`) by going through a third module system, commonjs (`require`). the module path that works for jscompiler and the bazel rules is ugly (e.g. `require('/js/error_collect_es6_sources.es6/node_modules/__main__/js/error')`), but it is enough to migrate the codebase to typescript.

mgechev · 2018-11-20T21:17:27Z

No updates since Jan. Can we close this PR?

googlebot added the cla: yes label Dec 20, 2017

mrmeku force-pushed the closure_ts_binary branch 4 times, most recently from 595d3ce to 3da243c Compare December 20, 2017 23:14

mrmeku mentioned this pull request Dec 20, 2017

Get JSCompiler to compile some TS code #74

Closed

mrmeku force-pushed the closure_ts_binary branch 24 times, most recently from 7da684f to 7f20002 Compare December 21, 2017 22:37

mrmeku force-pushed the closure_ts_binary branch 4 times, most recently from b973882 to c04b8d9 Compare December 31, 2017 21:09

pshields reviewed Jan 2, 2018

View reviewed changes

mrmeku force-pushed the closure_ts_binary branch 2 times, most recently from b8c3f63 to b1c87f7 Compare January 3, 2018 02:44

mrmeku force-pushed the closure_ts_binary branch from b1c87f7 to 6ed1a6d Compare January 3, 2018 03:03

mrmeku added 4 commits January 2, 2018 20:31

minor cleanup

c98516a

STRICT deps mode

13d0074

Use Vanilla Rules Closure

cd06710

Fix AngularJS example

dc1bffa

pshields reviewed Jan 3, 2018

View reviewed changes

mrmeku added 2 commits January 3, 2018 08:55

Allow user to specify their own angular bundles

50b6d23

STRICT deps mode in angularjs example

0c12c95

alexeagle reviewed Jan 5, 2018

View reviewed changes

More minor cleanup

8e30d3f

mrmeku force-pushed the closure_ts_binary branch from 1c40b5d to 8e30d3f Compare January 5, 2018 00:51

alexeagle reviewed Jan 5, 2018

View reviewed changes

codesuki mentioned this pull request Mar 5, 2018

interoperate with rules_closure #6

Closed

mrmeku closed this Feb 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added closure_ts_binary rules to interop ts_library with closure. #112

Added closure_ts_binary rules to interop ts_library with closure. #112

mrmeku commented Dec 20, 2017

pshields left a comment

pshields Jan 2, 2018

pshields Jan 2, 2018

mrmeku commented Jan 2, 2018 via email •

edited

Loading

pshields left a comment

pshields Jan 3, 2018

pshields Jan 3, 2018

alexeagle commented Jan 3, 2018

pshields commented Jan 4, 2018

alexeagle commented Jan 4, 2018

mrmeku commented Jan 4, 2018 •

edited

Loading

alexeagle Jan 5, 2018

alexeagle Jan 5, 2018

alexeagle Jan 5, 2018

alexeagle Jan 5, 2018

alexeagle Jan 5, 2018

alexeagle Jan 5, 2018

alexeagle Jan 5, 2018

alexeagle Jan 5, 2018

alexeagle Jan 5, 2018

alexeagle Jan 5, 2018

alexeagle Jan 5, 2018

alexeagle commented Jan 12, 2018

mgechev commented Nov 20, 2018

		@@ -0,0 +1,48 @@
		# Copyright 2017 The Bazel Authors. All rights reserved.

Added closure_ts_binary rules to interop ts_library with closure. #112

Added closure_ts_binary rules to interop ts_library with closure. #112

Conversation

mrmeku commented Dec 20, 2017

pshields left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrmeku commented Jan 2, 2018 via email • edited Loading

pshields left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexeagle commented Jan 3, 2018

pshields commented Jan 4, 2018

alexeagle commented Jan 4, 2018

mrmeku commented Jan 4, 2018 • 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

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 commented Jan 12, 2018

mgechev commented Nov 20, 2018

mrmeku commented Jan 2, 2018 via email •

edited

Loading

mrmeku commented Jan 4, 2018 •

edited

Loading