Skip to content
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

Bazel Generate Code for Transitive Dependencies #138

Merged
merged 1 commit into from
Nov 29, 2018
Merged

Bazel Generate Code for Transitive Dependencies #138

merged 1 commit into from
Nov 29, 2018

Conversation

Dig-Doug
Copy link
Contributor

@Dig-Doug Dig-Doug commented Nov 6, 2018

Updates the bazel typescript_proto_library rule to use an aspect to generate typescript files for the transitive dependencies of the proto.

This pull request builds on the work done here by @Globegitter to make the rule compatible with the rules_typescript library.

Summary of changes:

  • Updates typescript_proto_library to use an aspect
  • Bumps the rules_typescript version to the latest version
  • Renames all BUILD files to BUILD.bazel (the recommended name) and fixes formatting issues
  • Renames the rule in typescript_proto_dependencies from deps to ts_protoc_gen_deps so it doesn't conflict with other rules
  • Adds some tests under test/bazel for checking that the transitive files are included
    • NOTE: //test/bazel:pizza_service_proto_test_suite will fail until I can include the UMD modules for gprc-web-client and js-browser-headers, which depends on this pull request and this one.

Why do you need this?

Imagine you want to use :service_proto that has a string of dependencies to other protos:

proto_library(
  name = "service_proto",
  srcs = ["service.proto"],
  deps = [":message_proto"],
)

typescript_proto_library(
  name = "service_ts_proto",
  proto = ":service_proto",
)

proto_library(
  name = "message_proto",
  srcs = ["message.proto"],
  deps = [":sub_message_proto"],
)

# etc...

Currently, if you depend on the main :service_ts_proto, you also need to create and depend on typescript_proto_library rules for all of it's dependencies so that all of the necessary files are included in the final bundle. This becomes unmaintainable when you have a proto that transitively depends on 10s-100s of other files.

ts_library(
  name = "lib",
  deps = [
    ":service_ts_proto",
    ":message_ts_proto", # Needs to be created
    ":sub_message_ts_proto", # Needs to be created
    # etc...
  ],
)

This change updates the rule so that when you depend on :service_ts_proto, TS files are also generated for all dependencies of the service_proto rule and included automatically:

ts_library(
  name = "lib",
  deps = [
    ":service_ts_proto",
    # NOTE: You don't need :message_ts_proto or :sub_message_ts_proto unless you want to import them directly
  ],
)

API Changes / Compatibility Notes

typescript_proto_library changes:

  • Removed the debug attr: I don't think it is useful to consumers of the rule and rule authors can add logging while developing as needed

  • Removed the remove_dependencies attr: This attribute doesn't really make sense here because you can have two typescript_proto_library rules, each with different remove_dependencies values, that process the same proto_library. Dead code elimination can be handled by other tools.

defs.bzl Show resolved Hide resolved
@jonnyreeves
Copy link
Contributor

So had this PR floating around in the back of my mind recently, I was just wondering if there is any argument against extracting the bazel rules and accompanying tests into their own repository?

@Dig-Doug
Copy link
Contributor Author

Technically, I don't think there's any reason the rules couldn't be in another repository. However, I think you'd still want to have some of the bazel setup to build the protoc plugin in this repository.

@jonnyreeves
Copy link
Contributor

jonnyreeves commented Nov 26, 2018

@Dig-Doug the package-lock.json file is out of sync. Run npm install in the root of the repo to synchronise it, please

@Dig-Doug
Copy link
Contributor Author

Done, though the CI still fails. When I run ./generate.sh, nothing in the directory changes.

@jonnyreeves
Copy link
Contributor

@Dig-Doug the problem is still with your package-lock.json file, nothing that will be fixed by ./generate.sh (sorry the message is misleading).

Can you confirm you are using the node version specified in the .nvmrc file? Maybe your local version is out of sync and that's causing problems.

@Dig-Doug
Copy link
Contributor Author

The first time I ran nvm use + npm install the CI passed the generated files step. However, after trying to fix the later steps I've encountered the same issue again. This time though, the package.json file doesn't change, even with the correct node + npm version.

Any other ideas?

@jonnyreeves
Copy link
Contributor

jonnyreeves commented Nov 27, 2018

@Dig-Doug in 824c66f you modified 2 devDependencies in the package.json file, but there was no corrosponding change to the package-lock.json file; You need to run npm i in the root of the repo which should result in the package-lock.json file being sync'd.

@Dig-Doug
Copy link
Contributor Author

CI is passing now :)

The problem was that some of the urls were switching from http <-> https. I'm using the same node and npm version, so I'm not sure exactly why it's happening. I manually fixed the urls that changed and it's working now.

I also updated the bazel verson that's used, since it was very old and causing some issues.

Copy link
Contributor

@jonnyreeves jonnyreeves left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work, looking forward to landing this!

Do the bazel install instructions in the Readme need updating as a result of this change?

test/bazel/require.config.js Outdated Show resolved Hide resolved
travis-ci-build.sh Outdated Show resolved Hide resolved
@jonnyreeves jonnyreeves merged commit 14d69f6 into improbable-eng:master Nov 29, 2018
@Dig-Doug
Copy link
Contributor Author

Dig-Doug commented Dec 1, 2018

@jonnyreeves - Missed your comment about updating the readme, following up in #142

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants