feat: add js_rule_kind directive for per-group rule kind override#222
feat: add js_rule_kind directive for per-group rule kind override#222sallustfire wants to merge 3 commits intoaspect-build:mainfrom
Conversation
|
|
|
There was a problem hiding this comment.
Pull request overview
This PR adds a per-target-group rule-kind override mechanism to the JS/TS Gazelle extension via a new js_rule_kind directive, enabling generation of a canonical test rule kind (js_test) instead of relying on ts_project(testonly=True) for test groups.
Changes:
- Register
js_testas a first-class (static) kind with load metadata, import resolution, and dependency resolution support. - Add
# gazelle:js_rule_kind <group> [kind]directive parsing and config plumbing to override the generated kind per target group (with inheritance + reset). - Add new golden test cases covering default test-group override + inheritance and custom-group override combined with
map_kindremapping.
Reviewed changes
Copilot reviewed 19 out of 20 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| language/js/config.go | Adds js_rule_kind directive constant and per-group ruleKind config + setter. |
| language/js/configure.go | Parses js_rule_kind directive and applies it to a target group. |
| language/js/generate.go | Uses per-group kind override when generating source/test targets and adjusts testonly emission. |
| language/js/kinds.go | Introduces JsTestKind and wires it into kind metadata and apparent loads. |
| language/js/resolve.go | Includes js_test in import/resolve switch cases. |
| language/js/tests/rule_kind_override/** | New fixture verifying default test group override to js_test and inheritance to subpackages. |
| language/js/tests/rule_kind_custom_group/** | New fixture verifying custom group override to js_test plus map_kind remap to jest_test. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 20 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func (ts *typeScriptLang) addProjectRule(cfg *JsGazelleConfig, tsconfigRel string, tsconfig *typescript.TsConfig, args language.GenerateArgs, group *TargetGroup, targetName string, sourceFiles, genFiles, dataFiles []string, result *language.GenerateResult) (*rule.Rule, error) { | ||
| // Check for name-collisions with the rule being generated. | ||
| colError := ruleUtils.CheckCollisionErrors(targetName, TsProjectKind, sourceRuleKinds, args) | ||
| expectedKind := TsProjectKind | ||
| if group.ruleKind != "" { | ||
| expectedKind = group.ruleKind | ||
| } | ||
| colError := ruleUtils.CheckCollisionErrors(targetName, expectedKind, sourceRuleKinds, args) | ||
| if colError != nil { |
There was a problem hiding this comment.
expectedKind used for collision checks is set to ts_project unless a js_rule_kind override is present, but the actual generated kind may still become js_library when !hasTranspiledSources(info.sources). This can produce misleading collision error messages (reporting ts_project when the rule would be js_library). Consider computing expectedKind from the same logic as defaultKind (and then overriding with group.ruleKind), so collisions report the kind that will actually be generated.
There was a problem hiding this comment.
This is pre-existing behavior. The original code also hardcoded TsProjectKind here. The expectedKind is only used in the error message, not the collision logic itself. Fixing it to account for js_library (which requires inspecting source file extensions) is a separate concern and scope creep. Happy to entertain it in a follow-up if desired.
Register js_test as a static source rule kind so test target groups can
generate js_test instead of ts_project(testonly=True). The new
js_rule_kind directive selects the rule kind per target group, and
map_kind can further remap js_test to jest_test, vitest_test, etc.
Backwards compatible: default behavior unchanged. Opt-in via:
# gazelle:js_rule_kind {dirname}_tests js_test
# gazelle:map_kind js_test jest_test @aspect_rules_jest//jest:defs.bzl
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
sallustfire
force-pushed
the
feat/js-rule-kind-override
branch
from
7ae3dac to
a7bc543
Compare
| JsTestKind: { | ||
| MatchAny: false, | ||
| NonEmptyAttrs: map[string]bool{ | ||
| "srcs": true, |
There was a problem hiding this comment.
Isn't this incorrect?
js_test is just js_binary which has data and entry_point but no srcs or deps...?
|
Thanks for engaging here. This PR is a trial balloon to explore some DX changes to improve the JS/TS test ergonomics. I've got a fork of the plugin extending from this that's used in a 500k LoC TS codebase and it works well. Ideally, you can help me figure out what's upstreamable while keeping the plugin unopinionated yet flexible. In Go, Python, Rust, and Java, the test rule is the compilation/execution rule. JS/TS is different because the rules separate compilation ( Regardless of how the test runner works, you still want typechecking and first-class test targets without extra intermediate rules or hand-written boilerplate. The ecosystem is moving in a direction that makes this easier: runtimes increasingly execute Before and afterToday: Gazelle generates a compilation target. The test runner is manual, per-package: # Generated by gazelle
ts_project(
name = "widget_tests",
testonly = True,
srcs = ["widget.test.ts"],
deps = [":widget"],
)
# Hand-written in every package with tests
jest_test(
name = "test",
data = [":widget_tests"],
config = "//tools:jest.config.js",
node_modules = ":node_modules",
)With # Root BUILD (one-time setup):
# gazelle:js_rule_kind {dirname}_tests js_test
# gazelle:map_kind js_test jest_test //tools:defs.bzl
# Generated by gazelle. bazel test //widget:widget_tests works
jest_test(
name = "widget_tests",
srcs = ["widget.test.ts"],
deps = [":widget"],
)
|
Per-group rule kind override via
js_rule_kinddirectiveThe gap
The JS/TS gazelle extension generates
ts_project(testonly=True)for test files. This compiles tests but does not produce abazel test-able target. There is no way to get an executable test rule likejest_testorvitest_testfrom gazelle, analogous to how the Go extension generatesgo_testrather than justgo_library.The existing
map_kinddirective cannot solve this because it remaps all rules of a given kind. Writing# gazelle:map_kind ts_project jest_test //:defs.bzlwould remap both library and test targets, which is not the intent.The root cause is structural: the Go extension distinguishes
go_libraryfromgo_testas separate static kinds that can be independently remapped. The JS extension usests_projectfor both, distinguished only bytestonly = True.The fix
Register
js_test(from@aspect_rules_js//js:defs.bzl) as a static source rule kind, the test-target counterpart tots_project/js_library. A newjs_rule_kinddirective selects the rule kind per target group. Sincejs_testis statically registered inKinds()andApparentLoads(), the gazelle framework's resolver, merger, and load-statement generation all work without workarounds.Users who want a specific test runner combine this with the existing
map_kind:This is the same two-layer pattern the Go extension uses: gazelle emits a canonical kind (
go_test), users remap it to their preferred macro.Backwards compatible: default behavior is unchanged. No directive means
ts_project(testonly=True)as before.The directive inherits to subdirectories and can be reset in a child package with
# gazelle:js_rule_kind {dirname}_tests(group name only, no kind).Changes are visible to end-users: yes
js_rule_kind)Test plan
rule_kind_override-js_rule_kindon default test group with subdirectory inheritancerule_kind_custom_group-js_rule_kindon a custom group withmap_kindremapping tojest_test