Skip to content

build: update bazel typescript rules - #13408

Merged
vivian-hu-zz merged 1 commit into
angular:masterfrom
devversion:build/update-typescript-rules
Oct 4, 2018
Merged

build: update bazel typescript rules#13408
vivian-hu-zz merged 1 commit into
angular:masterfrom
devversion:build/update-typescript-rules

Conversation

@devversion

Copy link
Copy Markdown
Member
  • Updates the Bazel TypeScript rules and removes explicit dependency on the NodeJS rules (this is no longer necessary)
  • Fixes a test case which doesn't match the name of the upgrade data it runs with (css-names --> css-selectors)
  • Removes Bazel version check (the NodeJS rules automatically check the Bazel version based on the version of the NodeJS rules)

@devversiondevversion added pr: merge safe target: major This PR is targeted for the next major release labels Oct 3, 2018
@googlebotgooglebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Oct 3, 2018
Comment threadWORKSPACE
# `rules_typescript_dependencies()` would also load the NodeJS rules, but we specifically need
# at least v0.14.1 which includes: https://github.com/bazelbuild/rules_nodejs/pull/341
http_archive(
name = "build_bazel_rules_nodejs",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

tools/BUILD.bazel uses the nodejs_binary rule for the sass bundling custom rule, so I think we need to keep this.

@devversiondevversionOct 3, 2018

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Yeah, but rules_typescript also transitively loads the according rules_nodejs archive which is required for the TypeScript rules.

I'm not sure what's better. This is consistent with what's going on in angular/angular.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It's always better to explicitly depend on the things you use directly; in general depending on transitive dependencies is an anti-pattern.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

cc @alexeagle in case he has a comment on this

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I agree with that, just not sure how it would work the best with the TypeScript rules.

Based on my understanding, if we explicitly specify a version for rules_nodejs, we'll also overwrite the rules_nodejs version that comes with the TypeScript rules. This kind of feels like it could cause unexpected behavior if we just update one of the dependencies, but since we always do the updates manually, it shouldn't be a problem.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

you can supply an explicit rules_nodejs dependency, if you choose something too old, rules_typescript's ts_setup_workspace will throw an error

@devversiondevversionOct 4, 2018

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

@jelbourn Done. @alexeagle Cool, if it throws, that's perfect. Any reason why angular/angular uses the transitive dependency?

* Updates the Bazel TypeScript rules.
* Fixes a test case which doesn't match the name of the upgrade data it runs with (`css-names` --> `css-selectors`)
* Removes Bazel version check (the NodeJS rules automatically check the Bazel version based on the version of the NodeJS rules)
@devversion
devversionforce-pushed the build/update-typescript-rules branch from 4b0a42c to 50db44fCompareOctober 4, 2018 07:11

@jelbournjelbourn left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@jelbournjelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker labels Oct 4, 2018
@vivian-hu-zz
vivian-hu-zz merged commit 3254992 into angular:masterOct 4, 2018
roboshoes pushed a commit to roboshoes/material2 that referenced this pull request Oct 23, 2018
* Updates the Bazel TypeScript rules.
* Fixes a test case which doesn't match the name of the upgrade data it runs with (`css-names` --> `css-selectors`)
* Removes Bazel version check (the NodeJS rules automatically check the Bazel version based on the version of the NodeJS rules)
@angular-automatic-lock-bot

Copy link
Copy Markdown

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-botangular-automatic-lock-botBot locked and limited conversation to collaborators Sep 9, 2019
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

action: mergeThe PR is ready for merge by the caretakercla: yesPR author has agreed to Google's Contributor License Agreementtarget: majorThis PR is targeted for the next major release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@devversion@alexeagle@jelbourn@googlebot@vivian-hu-zz