Skip to content

Conversation

devversion
Copy link
Member

Migrates the @angular-devkit/architect package to the rules_js npm package rule, consuming the direct rules_ts output JS files.

Notably, substitution is FAR different than what it used to be with rules_nodejs, so we needed some extra work to leverage make_template for substitutions in package.json files. Keep in mind that for now, this does not apply to any other files; so we only substitute in the package.json, but not in e.g. .js files as before. We will follow-up on this.

The other jq merging/filtering for snapshot or tar references in package.json files is kept as is, and is temporarily duplicated. This is acceptable as the migration should be pretty smooth and quick.

@angular-robot angular-robot bot added the area: build & ci Related the build and CI infrastructure of the project label Jan 9, 2025
@devversion devversion force-pushed the migrate_npm_package branch 4 times, most recently from 711bbcb to 29b6e8f Compare January 9, 2025 15:36
Copy link
Member

@josephperrott josephperrott left a comment

Choose a reason for hiding this comment

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

LGTM

Migrates the `@angular-devkit/architect` package to the `rules_js` npm
package rule, consuming the direct `rules_ts` output JS files.

Notably, substitution is FAR different than what it used to be with
`rules_nodejs`, so we needed some extra work to leverage `make_template`
for substitutions in `package.json` files. **Keep in mind** that for
now, this does not apply to any other files; so we only substitute in
the `package.json`, but not in e.g. `.js` files as before. We will
follow-up on this.

The other jq merging/filtering for snapshot or tar references in
`package.json` files is kept as is, and is temporarily duplicated. This
is acceptable as the migration should be pretty smooth and quick.
@devversion devversion force-pushed the migrate_npm_package branch from 29b6e8f to 2c63962 Compare January 9, 2025 17:41
@devversion devversion force-pushed the migrate_npm_package branch from 67ea36f to fee3b95 Compare January 9, 2025 19:25
@devversion devversion requested a review from clydin January 9, 2025 19:34
@devversion devversion added action: merge The PR is ready for merge by the caretaker target: patch This PR is targeted for the next patch release labels Jan 9, 2025
@devversion devversion marked this pull request as ready for review January 9, 2025 19:34
out = "substituted_with_snapshot_repos/package.json",
)

expand_template(
Copy link
Member Author

Choose a reason for hiding this comment

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

This is newly added. The rest above is the same like before.

stamp_substitutions = get_npm_package_substitutions_for_rjs(),
)

_npm_package(
Copy link
Member Author

Choose a reason for hiding this comment

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

This is mostly the same as before too. Just that copy_files_to_directory is now _npm_package.

@devversion devversion added target: minor This PR is targeted for the next minor release target: rc This PR is targeted for the next release-candidate and removed target: patch This PR is targeted for the next patch release target: minor This PR is targeted for the next minor release labels Jan 10, 2025
@@ -0,0 +1,8 @@
load("//tools:interop.bzl", _ts_project = "ts_project")
Copy link
Collaborator

Choose a reason for hiding this comment

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

NIT: maybe a more meaningful filename? (defaults-interop) etc..

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, we'll rename that in a follow-up. Probably bazel/ts_project_interop.bzl

@alan-agius4 alan-agius4 merged commit b55bd31 into angular:main Jan 10, 2025
35 checks passed
@alan-agius4
Copy link
Collaborator

The changes were merged into the following branches: main, 19.1.x

@devversion
Copy link
Member Author

Proof that this change didn't affect npm output: angular/angular-devkit-architect-builds@83afe99

@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 Feb 10, 2025
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 target: rc This PR is targeted for the next release-candidate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants