Skip to content

Update rules nodejs #21928

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

Merged
merged 4 commits into from
Feb 22, 2021
Merged

Conversation

josephperrott
Copy link
Member

See individual commits.

@google-cla google-cla bot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Feb 16, 2021
Copy link
Member

@crisbeto crisbeto left a comment

Choose a reason for hiding this comment

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

LGTM once Jeremy's comments have been addressed.

@josephperrott josephperrott force-pushed the update-rules-nodejs branch 2 times, most recently from b64d8f4 to 7d0b9eb Compare February 17, 2021 21:32
@josephperrott josephperrott added target: patch This PR is targeted for the next patch release area: build & ci Related the build and CI infrastructure of the project area: cdk/schematics cdk/schematics is *not* a public API action: merge The PR is ready for merge by the caretaker labels Feb 17, 2021
@josephperrott
Copy link
Member Author

@crisbeto and @jelbourn I meant to create this as a draft PR initially, but mistakenly made it as ready for review.

I have cleaned it up and have everything passing. Go ahead and take a look again please.

Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -3,12 +3,12 @@
* want to launch any browser and just enable manual browser debugging.
*/

const bazelKarma = require('@bazel/karma');
const bazelKarma = require('@bazel/concatjs');
Copy link
Member

Choose a reason for hiding this comment

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

I find it really strange that they changed these rules to live under a package named for the bundler 😕

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, my guess is that its about consolidation overall: https://github.com/bazelbuild/rules_nodejs/wiki#introducing-bazelconcatjs

@josephperrott josephperrott added target: patch This PR is targeted for the next patch release and removed target: patch This PR is targeted for the next patch release labels Feb 17, 2021
Copy link
Member

@crisbeto crisbeto left a comment

Choose a reason for hiding this comment

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

Still LGTM with a couple of nits.

@josephperrott josephperrott force-pushed the update-rules-nodejs branch 2 times, most recently from e2faea4 to 5addaed Compare February 22, 2021 19:49
Update to use [email protected] and update correseponding build rule usages
and references
Loading the dev-infra rules via the @npm// workspace as the private
@npm_dev_infra_private repo is no longer published.
… type

Update JSON.parse usages to be better typed rather than resulting in an
any type.
… as a type

Update JSON.parse usages to be better typed rather than resulting in an
any type.
@josephperrott josephperrott removed request for a team, devversion and mmalerba February 22, 2021 20:45
@andrewseguin andrewseguin added target: minor This PR is targeted for the next minor release and removed target: patch This PR is targeted for the next patch release labels Feb 22, 2021
@andrewseguin andrewseguin merged commit 496867d into angular:master Feb 22, 2021
@josephperrott josephperrott deleted the update-rules-nodejs branch February 22, 2021 21:21
@@ -0,0 +1 @@
12.14.1
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any reason this can't just be 12? As is, it won't let me commit when I'm using NodeJS 12.21.0.

Copy link
Member Author

Choose a reason for hiding this comment

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

The intention is that by using the exact same version here and on CI, there are not any unexpected differences between our usages.

@angular-automatic-lock-bot
Copy link

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-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Apr 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker area: build & ci Related the build and CI infrastructure of the project area: cdk/schematics cdk/schematics is *not* a public API cla: yes PR author has agreed to Google's Contributor License Agreement target: minor This PR is targeted for the next minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants