feat(npm): enable symbolic macros for node_modules on Bazel 9+ - #2936
feat(npm): enable symbolic macros for node_modules on Bazel 9+#2936kirkobyte wants to merge 2 commits into
Conversation
Continues work from aspect-build#2666. The generated defs.bzl now wraps store invocations in `_all_stores` and link-package targets in `_npm_link_all_packages` symbolic macros when the Bazel version supports them (`macro()` global + `attr.label_list_dict`), which is Bazel 9+. On Bazel 7 and 8 the generator emits the original legacy-macro code unchanged, so there is no behavioral difference for those versions. Key fixes over the original PR (aspect-build#2666): - Version-gate via `bazel_features.globals.macro` and `hasattr(attr, "label_list_dict")` so Bazel 7/8 are not broken. - Fix first-party store template indentation for the macro impl body (4-space instead of 8-space). - Adjust `to_dict_attr` indent depth to match macro nesting level. Tested: `bazel build //:node_modules` passes on Bazel 7.7.1, 8.2.1, and 9.0.0. All `write_npm_translate_lock_*` snapshot tests pass on Bazel 7 (the repo's default version). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 29d234a0f3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if scope_targets: | ||
| for scope, scoped_targets in scope_targets.items(): | ||
| _js_library( | ||
| name = "node_modules/{}".format(scope), |
There was a problem hiding this comment.
On Bazel 9, any importer with scoped packages reaches this branch and declares targets such as node_modules/@fastify from a symbolic macro whose name is node_modules; Bazel's symbolic macro naming rule only permits targets equal to the macro name or prefixed with node_modules_, node_modules-, or node_modules., so these scoped aggregate targets are declared but cannot be built or depended on. This regresses existing supported uses such as the scope aggregate deps in npm/private/test/BUILD.bazel (:node_modules/@fastify, :node_modules/@figma, etc.), while the Bazel 7/8 legacy path remains unaffected.
Useful? React with 👍 / 👎.
Scoped aggregate targets like `node_modules/@types` use `/` as a separator, which is not allowed by Bazel's symbolic macro naming rules (only `_`, `-`, `.` are valid). This caused build failures on Bazel 9: Target //:node_modules/@types declared in symbolic macro 'node_modules' violates macro naming rules and cannot be built. Keep the `_all_stores` symbolic macro (its `.aspect_rules_` prefix is valid) but emit the scoped and `node_modules` js_library targets inline, the same way legacy macros do. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Closing — the |
|
So that would require breaking our
I wonder if we can convince the bazel team to add |
|
I imagine it's there because it can conflict with the other |
|
Scoped packages such as |
|
I’m meaning to refer to this concept and whether / is disallowed to remove label ambiguity: https://bazel.googlesource.com/bazel/+/5f083b8f5a073a3427d575aa54eac5b779226708/site/docs/guide.md
But I’d definitely avoid modifying all the paths if it is possible to allow / in the names |
2 participants
Summary
Continues work from #2666. On Bazel 9+, the generated
defs.bzlnow wraps store invocations and link-package targets in symbolic macros (_all_stores,_npm_link_all_packages), preparing for future lazy macro evaluation. On Bazel 7 and 8, the generator emits the original legacy-macro code unchanged.Changes are visible to end-users: yes
Release notes: On Bazel 9+,
npm_link_all_packagesnow uses symbolic macros for store invocations and link-package target creation, enabling visibility encapsulation and preparing for future lazy macro evaluation.What changed
bazel_features.globals.macroandhasattr(attr, "label_list_dict")to detect symbolic macro support. This activates on Bazel 9+ only (macro()exists on Bazel 8 butattr.label_list_dictdoes not).npm_translate_lockcode generator emits differentdefs.bzlcontent depending on the Bazel version:_all_stores = macro(...), link targets wrapped in_npm_link_all_packages = macro(...)_FP_STORE_MACRO_TMPLwith correct 4-space indentation for symbolic macro impl function bodies (fixes an indentation bug in the original PR).@bazel_features//:featurestonpm_translate_lock_generatebzl_library deps.Fixes over #2666
to_dict_attrindent depth to match macro nesting levelTest plan
bazel build //:node_modulespasses on Bazel 7.7.1, 8.2.1, and 9.0.0write_npm_translate_lock_*snapshot tests pass on Bazel 7 (the repo's default.bazelversion)