Skip to content

Post-merge-review: use scope manager for block-param tracking in template-no-obsolete-elements #2676

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 8 commits into from
Apr 19, 2026
Merged

Post-merge-review: use scope manager for block-param tracking in template-no-obsolete-elements #2676

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
029c2c0
Fix template-no-obsolete-elements: track element-level block params
johanrd Apr 12, 2026
5fddf2b
Document why manual block-param tracking is used instead of scope man…
johanrd Apr 13, 2026
44f81f7
Simplify comment: focus on HBS scope limitation, drop unverified GJS …
johanrd Apr 13, 2026
6189818
refactor(template-no-obsolete-elements): use scope manager for block …
johanrd Apr 15, 2026
c0df35f
Merge branch 'master' into night_fix/template-no-obsolete-elements
johanrd Apr 15, 2026
85e6988
chore: remove stray blank line after OBSOLETE array
johanrd Apr 15, 2026
a2ec4be
lint: hoist hasBindingInScopeChain to module scope, add curly braces
johanrd Apr 15, 2026
0c5ea45
chore(deps): bump ember-eslint-parser to ^0.10.0
johanrd Apr 19, 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
18 changes: 14 additions & 4 deletions lib/rules/template-no-obsolete-elements.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,12 +64,22 @@ module.exports = {
}
},
GlimmerElementNode(node) {
if (blockParamsInScope.includes(node.tag)) {
return;
}
if (obsolete.has(node.tag)) {
// Check the element's own tag before pushing its block params, so an
// element's own params don't shadow its own tag name (e.g.
// `<marquee as |marquee|>` should still flag the outer <marquee>).
if (!blockParamsInScope.includes(node.tag) && obsolete.has(node.tag)) {
context.report({ node, messageId: 'obsolete', data: { element: node.tag } });
}
// 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);
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.

shouldn't this be the responsibility of 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.

added a clarification above blockParamsInScope now.

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.

kinda seems like we should fix that rather than add a work-around here, ya?

Copy link
Copy Markdown
Contributor Author

@johanrd johanrd Apr 14, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

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.

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.

Switched to scope-manager (getScope(node.parent) + chain walk) as suggested. 36/37 tests pass; the one failing HBS test is blocked on ember-tooling/ember-eslint-parser#189

},
'GlimmerElementNode:exit'(node) {
const elementParams = node.blockParams || [];
for (let i = 0; i < elementParams.length; i++) {
blockParamsInScope.pop();
}
},
};
},
Expand Down
9 changes: 9 additions & 0 deletions tests/lib/rules/template-no-obsolete-elements.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,13 +11,22 @@ ruleTester.run('template-no-obsolete-elements', rule, {
`<template>{{#let (component 'whatever-here') as |plaintext|}}
<plaintext />
{{/let}}</template>`,
// Element-level block params (<Comp as |...|>) are now tracked
'<template><Comp as |plaintext|><plaintext /></Comp></template>',
'<template><Outer as |marquee|><marquee /></Outer></template>',
],
invalid: [
{
code: '<template><marquee></marquee></template>',
output: null,
errors: [{ messageId: 'obsolete' }],
},
// Element's own block params must not shadow its own tag name.
{
code: '<template><marquee as |marquee|></marquee></template>',
output: null,
errors: [{ messageId: 'obsolete', data: { element: 'marquee' } }],
},
],
});

Expand Down
Loading