Skip to content

fix(bazel/rules_angular): ensure correct typescript version is utilized in persistent worker runfiles#3598

Merged
alan-agius4 merged 2 commits intoangular:mainfrom
alan-agius4:worker-ts-version-resolve
Apr 10, 2026
Merged

fix(bazel/rules_angular): ensure correct typescript version is utilized in persistent worker runfiles#3598
alan-agius4 merged 2 commits intoangular:mainfrom
alan-agius4:worker-ts-version-resolve

Conversation

@alan-agius4
Copy link
Copy Markdown
Contributor

Updates the symlink_package rule to correctly propagate transitive source files, including standard library definitions, into the generated Bzlmod runfiles layout. Additionally, integrates the BazelSafeFilesystem instance within the persistent compiler worker to reliably resolve and load the workspace-provided TS toolchain at execution time.

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request refactors the Angular module extension to handle root module setup and uses labels for compiler and TypeScript dependencies. It also improves runfile collection for symlinked packages and introduces a BazelSafeFilesystem for worker execution. Review feedback suggests addressing potential path resolution issues in the sandbox environment, ensuring compatibility with older Node.js versions by avoiding import.meta.filename, and adding provider validation for the symlink rule.

@alan-agius4 alan-agius4 force-pushed the worker-ts-version-resolve branch 2 times, most recently from 2f6bcec to cc8d72a Compare April 10, 2026 09:34
@alan-agius4 alan-agius4 added the action: merge The PR is ready for merge by the caretaker label Apr 10, 2026
…ed in persistent worker runfiles

Updates the symlink_package rule to correctly propagate transitive source files, including standard library definitions, into the generated Bzlmod runfiles layout. Additionally, integrates the BazelSafeFilesystem instance within the persistent compiler worker to reliably resolve and load the workspace-provided TS toolchain at execution time.
@alan-agius4 alan-agius4 force-pushed the worker-ts-version-resolve branch from cc8d72a to 6cd21db Compare April 10, 2026 09:46
Copy link
Copy Markdown
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

Just one thing I want to make sure we think through, but happy with whatever decision you make on it.

angular_compiler_cli = attr.angular_compiler_cli,
typescript = attr.typescript,
)
if attr.name not in generated:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I understand we have control here so we can manage it, but does this mean that we we will just silently "fail" if someone tries to use the same name twice?

If dev-infra creates "dependency_rules_angular_configurable_deps" and then in framework we also create "rules_angular_configurable_deps", the dev-infra one would be encountered first and the framework one would get skipped? Would it make more sense to ensure that the same name is not generated twice?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes let me add an error message.

@alan-agius4 alan-agius4 merged commit 0844373 into angular:main Apr 10, 2026
13 checks passed
@alan-agius4
Copy link
Copy Markdown
Contributor Author

This PR was merged into the repository. The changes were merged into the following branches:

@alan-agius4 alan-agius4 deleted the worker-ts-version-resolve branch April 10, 2026 13:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

action: merge The PR is ready for merge by the caretaker

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants