-
Notifications
You must be signed in to change notification settings - Fork 95
Interop ts_library with closure_js_binary. #76
Conversation
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 |
internal/common/compilation.bzl
Outdated
@@ -329,3 +365,12 @@ def ts_providers_dict_to_struct(d): | |||
if type(value) == type({}): | |||
d[key] = struct(**value) | |||
return struct(**d) | |||
|
|||
def collect_js(ctx, deps): |
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.
This is extracted from rules_closure. What would you think about moving this around in the rules_closure repo to make it an importable function?
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.
Its heavily modified from the rules_closure implementation (and the rules closure implementation was within a directory named private). Also I'm not sure that its completely wise to make rules_typescript directly depend upon rules_closure since it'll cause user's of these rules to have to clone an entire repo they won't necessarily use
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.
+1 for copying in this case
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.
Done.
internal/common/compilation.bzl
Outdated
@@ -301,6 +329,14 @@ def compile_ts(ctx, | |||
collect_default=True, | |||
collect_data=True, | |||
), | |||
"closure_js_library": struct( |
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 know you like brevity, but when I'm implementing a spec, I want as much context as physically possible. Since we are implementing the rules_closure closure_js_library interface, I think we should leave the original comments along with a link to the original source.
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.
Donezo
1fc865e
to
9683ffe
Compare
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.
how does this PR relate to #74 ?
remote = "https://github.com/bazelbuild/rules_nodejs", | ||
tag = "0.1.11", | ||
commit = "f6eb823e1140da6eafbf77f56a249f72bb89416e", | ||
remote = "https://github.com/achew22/rules_nodejs", |
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 you have a separate PR to get that change upstream?
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.
Its already merged :)
internal/common/compilation.bzl
Outdated
root_without_prefix = root | ||
prefixes_to_remove = [ | ||
# Bazel < 0.8.0. | ||
"bazel-out/local-fastbuild/bin/", |
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.
use ctx.bin_bir.path
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.
Done
internal/common/compilation.bzl
Outdated
print(file_path) | ||
rooted_file = ctx.actions.declare_file(file_path) | ||
|
||
ctx.actions.run_shell( |
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.
note, we want to support Windows soon. I am not sure if this will work, it might rely on Posix cp
command?
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.
Donezo
internal/common/compilation.bzl
Outdated
@@ -329,3 +365,12 @@ def ts_providers_dict_to_struct(d): | |||
if type(value) == type({}): | |||
d[key] = struct(**value) | |||
return struct(**d) | |||
|
|||
def collect_js(ctx, deps): |
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.
+1 for copying in this case
internal/common/compilation.bzl
Outdated
@@ -329,3 +374,12 @@ def ts_providers_dict_to_struct(d): | |||
if type(value) == type({}): | |||
d[key] = struct(**value) | |||
return struct(**d) | |||
|
|||
def collect_js(ctx, deps): | |||
"""Aggregates transitive JavaScript source files from unfurled deps.""" |
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 have no idea what it means to unfurl a dep :)
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.
Removed!
e2b6102
to
f822a15
Compare
FYI: This PR was based off PR #74 (and includes the commit it contained) but fixes some bugs and cleans up the code a bit. |
3fd754b
to
e61df25
Compare
Months of teath nashing later...
e90829c
to
d5cf86f
Compare
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.
d5cf86f
to
fc2575b
Compare
We are switching back to PR #74 |
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.