Post-merge-review: fix template-no-passed-in-event-handlers ignore-config format and event list#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).
There was a problem hiding this comment.
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.
…event-handlers Removes the PascalCase-OR-@-OR-this.-OR-dot heuristic and the associated tests for <this.MyComponent>, <@someComponent>, <ns.Widget>, <my-button>, <custom-el>. Reverts to master's PascalCase-only gate. Better addressed by scope analysis once ember-eslint-parser exposes richer HBS scope — revisit as a separate PR then. Keeps the ignore-config format fix (argName without @) and the event list alignment with upstream.
johanrd
changed the title
template-no-passed-in-event-handlers: correct event list, fix ignore-config format, broaden component detection
2 participants
What's broken on master
Two independent issues:
ignore-config format broken for angle-bracket invocations. The port compares
ignoredAttrs.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.js L107–112) and explicitly rejects@-prefixed ignore entries as invalid config.Event list diverged from upstream. Master includes
mouseMove,mouseEnter,mouseLeave— these are native DOM events but don't have corresponding Ember classic-event method aliases (the mechanism the rule is protecting against). Upstream omits them.Fix
argName(the@-stripped form).mouseMove/mouseEnter/mouseLeavefromEMBER_EVENTSto align with upstream.Test plan
['@click']to['click'](the documented format). All 8 fail on master with the format fix reverted.mouseMove/mouseEnter/mouseLeaveare no longer flagged.Out of scope
The component-detection broadening (covering
<this.X>,<@x>,<ns.X>) was reverted — those forms are better addressed via scope analysis ifember-eslint-parserexposes richer HBS scope.