Skip to content

Extract rule: template-no-passed-in-event-handlers#2562

Merged
NullVoxPopuli merged 3 commits intoember-cli:masterfrom
NullVoxPopuli:nvp/template-lint-extract-rule-template-no-passed-in-event-handlers
Mar 22, 2026
Merged

Extract rule: template-no-passed-in-event-handlers#2562
NullVoxPopuli merged 3 commits intoember-cli:masterfrom
NullVoxPopuli:nvp/template-lint-extract-rule-template-no-passed-in-event-handlers

Conversation

@NullVoxPopuli
Copy link
Copy Markdown
Contributor

Split from #2371.

Copy link
Copy Markdown

@NullVoxPopuli-ai-agent NullVoxPopuli-ai-agent left a comment

Choose a reason for hiding this comment

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

Review: template-no-passed-in-event-handlers

Compared against the original ember-template-lint rule no-passed-in-event-handlers.

What looks good

  • The event handler set matches the original exactly (including mouseMove, mouseEnter, mouseLeave which the original groups under "Mouse events" rather than having a separate section).
  • Input and Textarea exclusions are correctly handled for both angle-bracket and curly invocations.
  • The ignore option schema is implemented and mirrors the original's config shape.
  • Both .gjs and .hbs parsers are tested, with good coverage of the valid/invalid cases from the original test suite.
  • originallyFrom metadata is present.

Items worth attention

  1. ignore config shape differs from original: The original rule's ignore config expects attribute names without the @ prefix (e.g., { ignore: { Foo: ['click'] } }), but the ESLint rule's schema/docs show values with the @ prefix (e.g., ['@click', '@submit']). The implementation checks ignoredAttrs.includes(attr.name) where attr.name includes @. This means the config format is intentionally different from the original. This is fine as a design choice, but worth calling out explicitly in the docs or a migration note so users porting configs know. The original even validates that ignore values do not start with @.

  2. Original also checks the isIgnored for curly MustacheStatement: The original rule's isIgnored is called for both ElementNode and MustacheStatement. In this PR, the GlimmerMustacheStatement handler does not check the ignore config at all. This means users cannot configure ignores for curly-style component invocations ({{foo click=...}}).

  3. Missing mouseMove, mouseEnter, mouseLeave in original's event set note: Minor -- the original has focusIn and focusOut listed in both the "Mouse events" and "Form events" sections (duplicated in the Set). Your implementation correctly has them only once in the Set, which is fine.

  4. No tests for the ignore option: The original test suite includes test cases for the ignore config (both angle-bracket and curly). The PR tests don't exercise the ignore option at all.

Suggestions

  • Add tests exercising the ignore option for both <Component> and {{component}} invocations.
  • Add ignore support to the GlimmerMustacheStatement handler to match the original.
  • Clarify in the docs that the ignore config uses @-prefixed names (unlike the original).

Overall a solid migration. The core detection logic is faithful to the original.

🤖 Automated review comparing with ember-template-lint source

NullVoxPopuli and others added 2 commits March 21, 2026 22:44
The `ignore` option was only applied to angle bracket invocations
(GlimmerElementNode) but not to curly-style invocations
(GlimmerMustacheStatement). This adds the same ignore lookup using
`path.original` as the component name for curly invocations.

Also adds tests for the `ignore` option covering both angle bracket
and curly invocations in both gjs and hbs test suites.

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
@NullVoxPopuli NullVoxPopuli force-pushed the nvp/template-lint-extract-rule-template-no-passed-in-event-handlers branch from bafc999 to cff1c83 Compare March 22, 2026 02:44
@NullVoxPopuli NullVoxPopuli enabled auto-merge March 22, 2026 03:20
@NullVoxPopuli NullVoxPopuli merged commit 4998048 into ember-cli:master Mar 22, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants