build: perform package.json substitutions in bazel build#22425
build: perform package.json substitutions in bazel build#22425dgp1130 merged 1 commit intoangular:masterfrom aspect-build:jq-substitutions
Conversation
| pkg_deps = [ | ||
| "//packages/angular_devkit/architect:package.json", | ||
| "//packages/angular_devkit/build_angular:package.json", | ||
| "//packages/angular_devkit/build_webpack:package.json", | ||
| "//packages/angular_devkit/core:package.json", | ||
| "//packages/angular_devkit/schematics:package.json", | ||
| "//packages/schematics/angular:package.json", | ||
| ], |
There was a problem hiding this comment.
When doing tar substitution we need the dependent package.json files so that we can get the package name and change:
"dependencies": {
"@some-package: "0.0.0",
}
to
"dependencies": {
"@some-package: "file:/path/to/pacakge.tar.gz",
}
There was a problem hiding this comment.
Unlike release and snapshot stamping, which just needs to look for 0.0.0 and replace that.
| # If set will replace dependency versions with tarballs for packages in this repo | ||
| bool_flag( | ||
| name = "enable_package_json_tar_deps", | ||
| build_setting_default = False, | ||
| ) | ||
|
|
||
| config_setting( | ||
| name = "package_json_use_tar_deps", | ||
| flag_values = { | ||
| ":enable_package_json_tar_deps": "true", | ||
| }, | ||
| ) |
There was a problem hiding this comment.
This is enabled by running yarn build:bazel --local. Once we flip to using the bazel script it will just be yarn build --local.
josephperrott
left a comment
josephperrott
left a comment
There was a problem hiding this comment.
LGTM
I think it looks good overall, but I want to have @devversion weigh in and see if he has any concerns or questions as this adds some complexity for substitution that we don't have in our other repositories.
josephperrott
added
action: review
josephperrott
left a comment
josephperrott
left a comment
There was a problem hiding this comment.
Discussed a little bit more with @devversion and I think we both are on the same page.
We have some concerns about using jq in the bzl file to modify the package.json
I think it would be more readable and become testable if we were to use a simple and easier to understand nodejs script that modifies the package.json file.
josephperrott
left a comment
josephperrott
left a comment
There was a problem hiding this comment.
LGTM
Approved to merge this as is and then follow up with additional testing and reorganization
devversion
left a comment
devversion
left a comment
There was a problem hiding this comment.
lgtm, we should make sure to keep track of the jq follow-up.
The takeaway from our meeting yesterday was that we should:
I'll be working on this next. Let me know if there's anything else you think I should add to that list. |
|
Updated with the above ^. |
devversion
left a comment
devversion
left a comment
There was a problem hiding this comment.
lgtm still, great to see a test for this. I added a few minor comments but other than that it seems ready to me.
| | del(.devDependencies) | ||
|
|
||
| # Add engines | ||
| + {"engines": {"node": "^12.20.0 || ^14.15.0 || >=16.10.0", "npm": "^6.11.0 || ^7.5.6 || >=8.0.0", "yarn": ">= 1.13.0"}} No newline at end of file |
There was a problem hiding this comment.
Should the engines really be configured here? personally that seems like a bad spot for me to have it placed. Would it make sense to have it somewhere else in a common spot for configuration of release output?
e.g. in Angular components we have a general config starlark file at the workspace top-level.
This could be a follow-up discussion, but that's just something I feel like could be improved IMO
There was a problem hiding this comment.
It looks like engines is set in the root package.json, but it was overridden in here: https://github.com/angular/angular-cli/blob/master/lib/packages.ts#L87
What if we just keep it in the root package.json and have that field override the project package.json in the filter?
Or would you prefer stamped substitutions like in the components repo?
There was a problem hiding this comment.
Good point. I think we cannot use the top-level engines field because that one prohobits the use of NPM, while actual consumers of the CLI should be fine to use NPM. Generally I like how we did the majority of versions within Angular Components (it's obviously not done for the engines, but I like that approach where a single place controls these common things; and it's not deeply hidden in some files).
Please feel free to leave this as is and discuss with @josephperrott and/or the CLI team.
There was a problem hiding this comment.
I think it makes sense to do substitutions for the node engine. I have a PR coming up for substitution-related things so probably more fitting to put it in there. I'll tag you when it's up.
There was a problem hiding this comment.
Yeah, I would love to do the substitutions if possible, but the follow up makes sense to me.
With the substitutions in place, we could have the template markers placed here in the JQ process and then use the starlark to consistently place the engine values.
| | del(.devDependencies) | ||
|
|
||
| # Add engines | ||
| + {"engines": {"node": "^12.20.0 || ^14.15.0 || >=16.10.0", "npm": "^6.11.0 || ^7.5.6 || >=8.0.0", "yarn": ">= 1.13.0"}} No newline at end of file |
There was a problem hiding this comment.
Yeah, I would love to do the substitutions if possible, but the follow up makes sense to me.
With the substitutions in place, we could have the template markers placed here in the JQ process and then use the starlark to consistently place the engine values.
devversion
added
action: merge
|
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. |
5 participants
@josephperrott @gregmagolan
This pr mimics the package.json substitutions that build.ts performs. The current build will not be affected by this change as we haven't yet flipped build.ts to use bazel.