Skip to content

Post-merge-review: fix template-no-passed-in-event-handlers ignore-config format and event list #2662

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
NullVoxPopuli merged 6 commits into from
Apr 16, 2026
Merged

Post-merge-review: fix template-no-passed-in-event-handlers ignore-config format and event list #2662

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
050d2bf
fix(template-no-passed-in-event-handlers): align event list and ignor…
johanrd Apr 12, 2026
f81b3a3
Document why mouseMove/mouseEnter/mouseLeave are excluded from event …
johanrd Apr 13, 2026
11c6e20
chore: remove 'upstream' references — describe behavior directly
johanrd Apr 14, 2026
434be3b
revert: drop component detection broadening in template-no-passed-in-…
johanrd Apr 15, 2026
76d23c1
chore: drop redundant what-comments
johanrd Apr 15, 2026
5f0dbe8
chore: restore 'Comprehensive Ember event handler names' comment
johanrd Apr 15, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 13 additions & 7 deletions lib/rules/template-no-passed-in-event-handlers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,6 @@ const EMBER_EVENTS = new Set([
'contextMenu',
'click',
'doubleClick',
'mouseMove',
Copy link
Copy Markdown
Contributor

@NullVoxPopuli NullVoxPopuli Apr 13, 2026

Choose a reason for hiding this comment

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

why were these removed?

Copy link
Copy Markdown
Contributor Author

@johanrd johanrd Apr 13, 2026

Choose a reason for hiding this comment

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

mouseMove, mouseEnter, and mouseLeave were removed to align with the upstream ember-template-lint no-passed-in-event-handlers list. Those three are native DOM events but don't have corresponding Ember classic-event method aliases (the mechanism the rule is protecting against).

'mouseEnter',
'mouseLeave',
'focusIn',
'focusOut',
'submit',
Expand Down Expand Up @@ -77,10 +74,19 @@ module.exports = {

return {
GlimmerElementNode(node) {
// Only check component invocations (PascalCase)
if (!/^[A-Z]/.test(node.tag)) {
// Only check component invocations. In GTS, dashed tags like <my-button>
Copy link
Copy Markdown
Contributor

@NullVoxPopuli NullVoxPopuli Apr 13, 2026

Choose a reason for hiding this comment

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

we might be able to check if something is a reference via the scope manager?

Copy link
Copy Markdown
Contributor Author

@johanrd johanrd Apr 13, 2026

Choose a reason for hiding this comment

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

yeah, though, is the rule even relevant in GJS/GTS? The pattern it guards against (@OnClick={{handler}} passed to a classic component's event method) is a classic component convention. In strict mode all components are Glimmer components and there's no classic event system (right?), so no component would consume these args that way. Should this perhaps be templateMode: 'loose' (HBS-only)?

Copy link
Copy Markdown
Contributor

@NullVoxPopuli NullVoxPopuli Apr 14, 2026

Choose a reason for hiding this comment

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

it's still relevant yes, as a classic is just a compoennt that extends from @ember/component, which can be used in strict mode

Copy link
Copy Markdown
Contributor Author

@johanrd johanrd Apr 14, 2026

Choose a reason for hiding this comment

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

okay, good point.

we might be able to check if something is a reference via the scope manager?

To my understanding, scope does work for direct tag references in GTS — scope.references would resolve <Foo> if Foo is imported. But the three forms this PR specifically adds (<this.MyComponent>, <@someComponent>, <ns.Widget>) are property accesses and argument references, not direct variable bindings, so scope analysis doesn't reach them even in GTS.

In HBS mode the scope manager is intentionally empty, so scope alone can't carry the full check in either case. The current heuristic covers all four forms in both modes. We could layer a scope check on top for the GTS direct-reference case? (even if it wouldn't replace the heuristic fully?). not sure at all.

Copy link
Copy Markdown
Contributor

@NullVoxPopuli NullVoxPopuli Apr 15, 2026

Choose a reason for hiding this comment

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

sounds like we have a bug in our scope implementation that we need to fix.

We could layer a scope check on top for the GTS direct-reference case? (even if it wouldn't replace the heuristic fully?). not sure at all.

It could replace the heuristic fully (because component is not defined by casing, but by <syntax>, like... you don't know that <div> isn't a component without scope analysis). the heuristic is only needed when scope would not be available (and is imperfect due to app tree merging).

Copy link
Copy Markdown
Contributor Author

@johanrd johanrd Apr 15, 2026

Choose a reason for hiding this comment

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

I wasn't able to solve 'replacing the heuristic fully' so I just reverted the component-detection broadening from this PR to move on the other parts.

// are HTML (imports can't have dashes); lowercase non-dashed tags are
// HTML. Components are PascalCase, @-prefixed args, this.-prefixed
// references, or dot-paths (re-exports).
const isComponent =
/^[A-Z]/.test(node.tag) ||
node.tag.startsWith('@') ||
node.tag.startsWith('this.') ||
node.tag.includes('.');
if (!isComponent) {
return;
}

// Skip built-in Input/Textarea
if (node.tag === 'Input' || node.tag === 'Textarea') {
return;
Expand All @@ -98,8 +104,8 @@ module.exports = {
}
const argName = attr.name.slice(1);

// Check ignore config
if (ignoredAttrs.includes(attr.name)) {
// Check ignore config (use bare name without @)
if (ignoredAttrs.includes(argName)) {
continue;
}

Expand Down
47 changes: 32 additions & 15 deletions tests/lib/rules/template-no-passed-in-event-handlers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,15 @@ ruleTester.run('template-no-passed-in-event-handlers', rule, {
'<template><Foo @random={{true}} /></template>',
'<template><Input @click={{this.handleClick}} /></template>',
'<template><Textarea @click={{this.handleClick}} /></template>',

// HTML elements are not checked (in GTS <my-button> is HTML, not a component)
'<template><my-button @click={{this.handleClick}} /></template>',
'<template><custom-el @submit={{this.handleSubmit}} /></template>',

// mouseMove/mouseEnter/mouseLeave are NOT in upstream's event list
'<template><Foo @mouseMove={{this.handleMove}} /></template>',
'<template><Foo @mouseEnter={{this.handleEnter}} /></template>',
'<template><Foo @mouseLeave={{this.handleLeave}} /></template>',
'<template>{{foo}}</template>',
'<template>{{foo onClick=this.handleClick}}</template>',
'<template>{{foo onclick=this.handleClick}}</template>',
Expand All @@ -48,11 +57,11 @@ ruleTester.run('template-no-passed-in-event-handlers', rule, {
// ignore option — angle bracket invocation
{
code: '<template><Foo @click={{this.handleClick}} /></template>',
options: [{ ignore: { Foo: ['@click'] } }],
options: [{ ignore: { Foo: ['click'] } }],
},
{
code: '<template><Foo @click={{this.handleClick}} @submit={{this.handleSubmit}} /></template>',
options: [{ ignore: { Foo: ['@click', '@submit'] } }],
options: [{ ignore: { Foo: ['click', 'submit'] } }],
},

// ignore option — curly invocation
Expand Down Expand Up @@ -82,25 +91,33 @@ ruleTester.run('template-no-passed-in-event-handlers', rule, {
errors: [{ messageId: 'unexpected' }],
},
{
code: `<template>
<CustomButton @mouseEnter={{this.handleHover}} />
</template>`,
code: '<template><Foo @click={{this.handleClick}} /></template>',
output: null,
errors: [{ messageId: 'unexpected' }],
},

{
code: '<template><Foo @click={{this.handleClick}} /></template>',
code: '<template><Foo @keyPress={{this.handleClick}} /></template>',
output: null,
errors: [{ messageId: 'unexpected' }],
},
{
code: '<template><Foo @keyPress={{this.handleClick}} /></template>',
code: '<template><Foo @submit={{this.handleClick}} /></template>',
output: null,
errors: [{ messageId: 'unexpected' }],
},
// Non-PascalCase component forms: this.-prefixed, @-prefixed, dot-path
{
code: '<template><Foo @submit={{this.handleClick}} /></template>',
code: '<template><this.MyComponent @click={{this.handleClick}} /></template>',
output: null,
errors: [{ messageId: 'unexpected' }],
},
{
code: '<template><@someComponent @click={{this.handleClick}} /></template>',
output: null,
errors: [{ messageId: 'unexpected' }],
},
{
code: '<template><ns.Widget @submit={{this.handleSubmit}} /></template>',
output: null,
errors: [{ messageId: 'unexpected' }],
},
Expand All @@ -124,14 +141,14 @@ ruleTester.run('template-no-passed-in-event-handlers', rule, {
{
code: '<template><Bar @click={{this.handleClick}} /></template>',
output: null,
options: [{ ignore: { Foo: ['@click'] } }],
options: [{ ignore: { Foo: ['click'] } }],
errors: [{ messageId: 'unexpected' }],
},
// ignore option — only ignores specified attrs (angle bracket)
{
code: '<template><Foo @submit={{this.handleSubmit}} /></template>',
output: null,
options: [{ ignore: { Foo: ['@click'] } }],
options: [{ ignore: { Foo: ['click'] } }],
errors: [{ messageId: 'unexpected' }],
},
// ignore option — only ignores specified component (curly)
Expand Down Expand Up @@ -183,11 +200,11 @@ hbsRuleTester.run('template-no-passed-in-event-handlers', rule, {
// ignore option — angle bracket invocation
{
code: '<Foo @click={{this.handleClick}} />',
options: [{ ignore: { Foo: ['@click'] } }],
options: [{ ignore: { Foo: ['click'] } }],
},
{
code: '<Foo @click={{this.handleClick}} @submit={{this.handleSubmit}} />',
options: [{ ignore: { Foo: ['@click', '@submit'] } }],
options: [{ ignore: { Foo: ['click', 'submit'] } }],
},

// ignore option — curly invocation
Expand Down Expand Up @@ -266,14 +283,14 @@ hbsRuleTester.run('template-no-passed-in-event-handlers', rule, {
{
code: '<Bar @click={{this.handleClick}} />',
output: null,
options: [{ ignore: { Foo: ['@click'] } }],
options: [{ ignore: { Foo: ['click'] } }],
errors: [{ messageId: 'unexpected' }],
},
// ignore option — only ignores specified attrs (angle bracket)
{
code: '<Foo @submit={{this.handleSubmit}} />',
output: null,
options: [{ ignore: { Foo: ['@click'] } }],
options: [{ ignore: { Foo: ['click'] } }],
errors: [{ messageId: 'unexpected' }],
},
// ignore option — only ignores specified component (curly)
Expand Down
Loading