Post-merge-review: Fix template-no-obsolete-elements: track element-level block params#2676
Post-merge-review: Fix template-no-obsolete-elements: track element-level block params#2676johanrd wants to merge 3 commits intoember-cli:masterfrom
template-no-obsolete-elements: track element-level block params#2676Conversation
The rule suppressed obsolete-element reports when the tag name shadows a
block param from a GlimmerBlockStatement (e.g. {{#let (...) as |marquee|}}).
It did not track element-level block params (<Comp as |marquee|>), so
inside an angle-bracket component the shadowing was ignored and the tag
was falsely reported. Push element block params on enter and pop on exit,
mirroring the existing GlimmerBlockStatement handling.
johanrd
force-pushed
the
night_fix/template-no-obsolete-elements
branch
from
511b615 to
029c2c0
Compare
| // Element-level block params (e.g. `<Comp as |param|>`) are scoped to | ||
| // the children, so push them after the obsolete check. Pop on exit. | ||
| const elementParams = node.blockParams || []; | ||
| blockParamsInScope.push(...elementParams); |
There was a problem hiding this comment.
shouldn't this be the responsibility of the scope manager?
There was a problem hiding this comment.
added a clarification above blockParamsInScope now.
There was a problem hiding this comment.
kinda seems like we should fix that rather than add a work-around here, ya?
There was a problem hiding this comment.
yes, maybe, although I'm not sure how to approach that.
To my understanding, getScope() runs into two problems:
HBS mode: ember-eslint-parser's HBS path creates an intentionally empty scope manager — Glimmer block params from {{#let ... as |x|}} and <Comp as |x|> are not registered, so getScope() sees nothing.
GTS mode: at the GlimmerElementNode visitor, the element's own as |x| params are already visible in scope. So getScope() at <marquee as |marquee|> test would find marquee in scope and incorrectly suppress the flag. The push-after-check ordering handles this: the element is checked against outer params only, then its own are pushed for its children.
or were you thinking something else completely? upstream to ember-eslint-parser? thanks.
2 participants
What's broken on
masterThe rule suppresses reports when the tag name shadows a block param from a
GlimmerBlockStatement(e.g.{{#let (...) as |marquee|}}<marquee />). It does not track element-level block params (<Comp as |marquee|>), so angle-bracket shadowing is ignored and the inner<marquee />is falsely reported.Fix
Push element block params on
GlimmerElementNodeenter, pop on exit, mirroring the existingGlimmerBlockStatementhandling.Test plan
36/36 tests pass. 2 new valid tests (element-level shadowing of
plaintextandmarquee) fail on master.Co-written by Claude.