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

fix(typescript): propagate tags to validate_options #3260

Merged
merged 2 commits into from
Jan 18, 2022

Conversation

djmarcin
Copy link
Contributor

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature (please, look at the "Scope of the project" section in the README.md file)
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Other... Please describe:

What is the current behavior?

Issue Number: #3259

What is the new behavior?

Tags are propagated to the validate_options rule

Does this PR introduce a breaking change?

  • Yes
  • No

Strictly, the answer here might be yes, particularly if someone was doing something very esoteric with custom tags and relying on the fact that propagation to this rule was not happening, but this seems extremely unlikely.

Other information

Copy link
Collaborator

@alexeagle alexeagle left a comment

Choose a reason for hiding this comment

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

Thanks!

Do you think we might want testonly and visibility to be propagated as well? Just thinking to be consistent with https://github.com/bazelbuild/rules_nodejs/blob/5.x/packages/typescript/index.bzl#L507-L511

@djmarcin
Copy link
Contributor Author

Thanks!

Do you think we might want testonly and visibility to be propagated as well? Just thinking to be consistent with https://github.com/bazelbuild/rules_nodejs/blob/5.x/packages/typescript/index.bzl#L507-L511

Seems reasonable. The target is named with a leading underscore (_validate...) so it seems like it's not intended to be depended on by other things, making testonly and visibility likely extraneous but it probably doesn't hurt.

@djmarcin
Copy link
Contributor Author

@alexeagle I moved common_kwargs up and reused it for validate_options. It seems like this may also be good to pass to write_tsconfig and nodejs_binary, but I did not do that here as it expands the scope of the PR a bit. The write_tsconfig macro needs to be further modified to take & pass along **kwargs to the underlying rule. Let me know if I should add that here.

@alexeagle
Copy link
Collaborator

buildifier failed - I'd suggest running pre-commit install in this repo so a git pre-commit hook will always format your starlark.

@djmarcin
Copy link
Contributor Author

buildifier failed - I'd suggest running pre-commit install in this repo so a git pre-commit hook will always format your starlark.

fixed & force pushed the last commit (not sure what the convention for this repo is in terms of the structured commit messages and trivial typo fixes like that...)

@alexeagle alexeagle merged commit e8b2e8f into bazel-contrib:stable Jan 18, 2022
@djmarcin djmarcin deleted the ts-project-propagate branch January 18, 2022 03:49
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.

2 participants