Skip to content

/ components

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

Jump to bottom

Update rules nodejs #21928

Merged
merged 4 commits into from Feb 22, 2021
Merged

Update rules nodejs #21928

merged 4 commits into from Feb 22, 2021

Conversation

@josephperrott
Copy link
Member

@josephperrott josephperrott commented Feb 16, 2021

See individual commits.
@josephperrott josephperrott requested review from devversion, jelbourn and angular/dev-infra-components as code owners Feb 16, 2021
@google-cla google-cla bot added the cla: yes label Feb 16, 2021
@jelbourn
jelbourn reviewed Feb 17, 2021
View changes
WORKSPACE Outdated Show resolved Hide resolved
tools/highlight-files/BUILD.bazel Outdated Show resolved Hide resolved
tools/postinstall/apply-patches.js Outdated Show resolved Hide resolved
test/bazel-karma-local-config.js Outdated Show resolved Hide resolved
packages.bzl Show resolved Hide resolved
@crisbeto
crisbeto approved these changes Feb 17, 2021
View changes
Copy link
Member

@crisbeto crisbeto left a comment

LGTM once Jeremy's comments have been addressed.
@josephperrott josephperrott force-pushed the josephperrott:update-rules-nodejs branch from 5c84d21 to fee339e Feb 17, 2021
@josephperrott josephperrott requested a review from mmalerba as a code owner Feb 17, 2021
@josephperrott josephperrott force-pushed the josephperrott:update-rules-nodejs branch 2 times, most recently from b64d8f4 to 7d0b9eb Feb 17, 2021
@josephperrott josephperrott requested review from jelbourn and crisbeto Feb 17, 2021
@josephperrott
Copy link
Member Author

@josephperrott josephperrott commented Feb 17, 2021

@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.
@jelbourn
jelbourn approved these changes Feb 17, 2021
View changes
Copy link
Member

@jelbourn jelbourn left a comment

LGTM
test/bazel-karma-local-config.js
@@ -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');

This comment has been minimized.

Sign in to view
@jelbourn

jelbourn Feb 17, 2021
Member

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

This comment has been minimized.

Sign in to view
@josephperrott

josephperrott Feb 17, 2021
Author Member

Yeah, my guess is that its about consolidation overall: https://github.com/bazelbuild/rules_nodejs/wiki#introducing-bazelconcatjs
@crisbeto
crisbeto approved these changes Feb 18, 2021
View changes
Copy link
Member

@crisbeto crisbeto left a comment

Still LGTM with a couple of nits.
package.json Outdated Show resolved Hide resolved
src/cdk/schematics/ng-add/index.spec.ts Outdated Show resolved Hide resolved
@josephperrott josephperrott force-pushed the josephperrott:update-rules-nodejs branch 2 times, most recently from e2faea4 to 5addaed Feb 19, 2021
@josephperrott josephperrott force-pushed the josephperrott:update-rules-nodejs branch from 4623415 to 54834e2 Feb 22, 2021
@josephperrott
build: update to rules_nodejs@3.1.0
Loading status checks…
f409b73 
Update to use rules_nodejs@3.1.0 and update correseponding build rule usages
and references
@josephperrott
build: update load statements to load dev-infra via @npm// workspace
Loading status checks…
4d0e062 
Loading the dev-infra rules via the @npm// workspace as the private
@npm_dev_infra_private repo is no longer published.
@josephperrott
fix(cdk/schematics): update JSON.parse usages to cast the result as a…
Loading status checks…
512690b 
… type

Update JSON.parse usages to be better typed rather than resulting in an
any type.
@josephperrott
fix(material/schematics): update JSON.parse usages to cast the result…
Loading status checks…
6676d3e 
… as a type

Update JSON.parse usages to be better typed rather than resulting in an
any type.
@josephperrott josephperrott force-pushed the josephperrott:update-rules-nodejs branch from 54834e2 to 6676d3e Feb 22, 2021
@josephperrott josephperrott removed request for angular/dev-infra-components, devversion and mmalerba Feb 22, 2021
@andrewseguin andrewseguin merged commit 496867d into angular:master Feb 22, 2021
16 checks passed
16 checks passed
@in-solidarity
Inclusive Language Check completed with success
Details
@ngbot
ci/angular: merge status All checks passed!
ci/circleci: api_golden_checks Your tests passed on CircleCI!
Details
ci/circleci: bazel_build Your tests passed on CircleCI!
Details
ci/circleci: build_release_packages Your tests passed on CircleCI!
Details
ci/circleci: e2e_tests Your tests passed on CircleCI!
Details
ci/circleci: integration_tests Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: ngcc_compatibility Your tests passed on CircleCI!
Details
ci/circleci: tests_browserstack Your tests passed on CircleCI!
Details
ci/circleci: tests_local_browsers Your tests passed on CircleCI!
Details
ci/circleci: tests_saucelabs Your tests passed on CircleCI!
Details
ci/circleci: upload_release_packages Your tests passed on CircleCI!
Details
ci/circleci: view_engine_build Your tests passed on CircleCI!
Details
ci/circleci: view_engine_test Your tests passed on CircleCI!
Details
@google-cla
cla/google All necessary CLAs are signed
@josephperrott josephperrott deleted the josephperrott:update-rules-nodejs branch Feb 22, 2021
@Splaktar
Splaktar reviewed Mar 11, 2021
View changes
.nvmrc
@@ -0,0 +1 @@
12.14.1

This comment has been minimized.

Sign in to view
@Splaktar

Splaktar Mar 11, 2021
Member

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.

This comment has been minimized.

Sign in to view
@josephperrott

josephperrott Mar 11, 2021
Author Member

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

@angular-automatic-lock-bot angular-automatic-lock-bot bot commented Apr 11, 2021

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.
Reviewers

@Splaktar Splaktar

@crisbeto crisbeto

@jelbourn jelbourn

Assignees
No one assigned
Labels
area: build area: cdk/schematics cla: yes merge ready merge safe target: minor
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
@josephperrott @jelbourn @Splaktar @crisbeto @andrewseguin