build: switch Node.js toolchain to derive version from .nvmrc. [main] #32575
Conversation
Remove hardcoded node.js version
alan-agius4
added
target: patch
angular-robot
bot
added
the
area: build & ci
alan-agius4
added
target: minor
alan-agius4
changed the title
Ensure that the same version of pnpm is used.
| use_repo(node, "nodejs_toolchains") | ||
|
|
||
| pnpm = use_extension("@aspect_rules_js//npm:extensions.bzl", "pnpm") | ||
| pnpm.pnpm( |
There was a problem hiding this comment.
Why are we updating the pnpm version as well? If we just use the default, it still runs the version we define in our package.json anyway and then we don't have to maintain another location and keep them in sync.
There was a problem hiding this comment.
Does it? The default is pnpm version 8 is rules node.js and is not auto inferred from the engines field.
There was a problem hiding this comment.
I did automate the syncing part:
There was a problem hiding this comment.
If we are going to start inferring the node version from .nvmrc why not infer pnpm from packageManager in package.json?
There was a problem hiding this comment.
There are two problems with that.
1: rules_js doesn’t have a full list of pnpm versions (currently the latest available is 10.21.0)
2. Whenever we cut a release we need to also update the bazel lock file, because the hash of the contents of the package.json that is in the bazel lock file will be invalidated.
Same problems/approach we use the TS versions.
There was a problem hiding this comment.
rules_js doesn’t have a full list of pnpm versions
I think that is true for node version as well, and I think it might be a limitation we can accept.
Whenever we cut a release we need to also update the bazel lock file.
If this is happening in multiple places now, maybe we should just update the bazel lock file during release. It seems more straightforward to do this one update than us having to set up of multiple pieces of tooling to update and monitor the versions.
What do you think?
There was a problem hiding this comment.
That will still not be enough, since rules_ts/rules_js doesn’t check available version of “pnpm” and “typescript”, on NPM but against an internal list. The integrity value needs to be passed explicitly.
https://github.com/aspect-build/rules_js/blob/main/npm/private/versions.bzl
https://github.com/aspect-build/rules_ts/blob/main/ts/private/versions.bzl
There was a problem hiding this comment.
Happy to drop the last commit if you want to discuss this more after the break.
There was a problem hiding this comment.
Why don't we merge this as it is and we can have this discussion within the tooling pull request in dev-infra where the management of the versions would actually be handled.
| use_repo(node, "nodejs_toolchains") | ||
|
|
||
| pnpm = use_extension("@aspect_rules_js//npm:extensions.bzl", "pnpm") | ||
| pnpm.pnpm( |
There was a problem hiding this comment.
Why don't we merge this as it is and we can have this discussion within the tooling pull request in dev-infra where the management of the versions would actually be handled.
alan-agius4
added
action: merge
|
This PR was merged into the repository. The changes were merged into the following branches:
|
3 participants
Remove hardcoded node.js version