-
Notifications
You must be signed in to change notification settings - Fork 6.8k
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
Update rules nodejs #21928
Conversation
There was a problem hiding this 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.
5c84d21
to
fee339e
Compare
b64d8f4
to
7d0b9eb
Compare
There was a problem hiding this 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'); |
There was a problem hiding this comment.
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 😕
There was a problem hiding this comment.
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
There was a problem hiding this 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.
e2faea4
to
5addaed
Compare
4623415
to
54834e2
Compare
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.
54834e2
to
6676d3e
Compare
@@ -0,0 +1 @@ | |||
12.14.1 |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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.
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
See individual commits.