-
Notifications
You must be signed in to change notification settings - Fork 524
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor: move ts_proto_library to @bazel/labs #1159
Conversation
7b51f9d
to
202468e
Compare
|
||
def npm_bazel_labs_dependencies(): | ||
yarn_install( | ||
name = "build_bazel_rules_typescript_protobufs_compiletime_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.
nit: poorly named as it is now in build_bazel_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.
also, we could probably remove this extra yarn_install entirely by moving its deps to the @bazel/labs package.json but not a big deal as its in labs which is experimental code anyway
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.
yeah also I seem to recall this workspace name appeared in a few places which is why we didn't rename it last time we moved it. Going for a minimal edit here to clear out the 1.0 space and yeah, don't mind a mess in labs
@@ -69,8 +70,24 @@ nodejs_binary( | |||
install_source_map_support = False, | |||
) | |||
|
|||
# Runtime libraries needed by the protobufjs library. | |||
# Any JS code produced by the ts_proto_library rule has a runtime dependency on these scripts. | |||
js_library( |
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.
hmmmmmm.. looks like js_library has the additional amd_names attribute which is being used. will note that in #149
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.
sigh, yes that needs a new home
@@ -28,23 +28,3 @@ def ts_setup_workspace(): | |||
|
|||
# 0.16.8: ng_package fix for packaging binary files | |||
check_rules_nodejs_version("0.16.8") | |||
|
|||
yarn_install( | |||
name = "build_bazel_rules_typescript_devserver_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.
🌮 🌮 🌮 🌮 🌮 🌮
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.
LGTM
We don't intend to fully support this for 1.0 Also remove a nested install, by vendoring the one file we need for ts_devserver Fixes bazel-contrib#1152 Fixes bazel-contrib#1151
202468e
to
bae9c8e
Compare
We don't intend to fully support this for 1.0
Also remove a nested install, by vendoring the one file we need for ts_devserver
Fixes #1152
Fixes #1151