Post-merge-review: Fix template-no-passed-in-event-handlers: correct event list, fix ignore-config format, broaden component detection#2662
Conversation
…e config with upstream - Remove mouseMove, mouseEnter, mouseLeave from event list (not in upstream) - Use bare names (without @) in ignore config for angle bracket invocations - Remove unnecessary PascalCase gate on ElementNode visitor
johanrd
force-pushed
the
night_fix/template-no-passed-in-event-handlers
branch
from
be7fce9 to
050d2bf
Compare
| 'contextMenu', | ||
| 'click', | ||
| 'doubleClick', | ||
| 'mouseMove', |
There was a problem hiding this comment.
why were these removed?
There was a problem hiding this comment.
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).
| GlimmerElementNode(node) { | ||
| // Only check component invocations (PascalCase) | ||
| if (!/^[A-Z]/.test(node.tag)) { | ||
| // Only check component invocations. In GTS, dashed tags like <my-button> |
There was a problem hiding this comment.
we might be able to check if something is a reference via the scope manager?
There was a problem hiding this comment.
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)?
There was a problem hiding this comment.
it's still relevant yes, as a classic is just a compoennt that extends from @ember/component, which can be used in strict mode
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
2 participants
What's broken on
masterTwo independent issues:
ignoreconfig format broken for angle-bracket invocations. The port comparesignoredAttrs.includes(attr.name)whereattr.name === '@click'. The docs say to configure{ Foo: ['click'] }(bare name, no@) — but with the current code that never matches. Upstream slices the@off first (no-passed-in-event-handlers.jsL107–112) and explicitly rejects@-prefixed ignore entries as invalid config (L58)./^[A-Z]/.test(node.tag)gate silently skips<this.X>,<@x>, and<ns.X>— all valid GTS component invocations per the template tag format guide. The port needs some HTML-vs-component discrimination because it has no JS scope tracker.Fix
argName(the@-stripped form).@OR starts withthis.OR contains.. Lowercase/kebab-case tags (<my-button>) are treated as HTML — correct in GTS (imports can't have dashes) and a reasonable approximation in HBS (@arg={{handler}}on a true HTML element is nonsensical).Test plan
<this.MyComponent>,<@someComponent>,<ns.Widget>fail on master (PascalCase gate silently skips them)<my-button>/<custom-el>confirm HTML elements are not checked['@click']to['click'](the documented format). All 8 fail on master with the format fix reverted.Co-written by Claude.