Skip to content

Post-merge-review: extend allowlist with svg-tags on template-no-block-params-for-html-elements#2689

Open
johanrd wants to merge 2 commits intoember-cli:masterfrom
johanrd:day_fix/template-no-block-params-for-html-elements
Open

Post-merge-review: extend allowlist with svg-tags on template-no-block-params-for-html-elements#2689
johanrd wants to merge 2 commits intoember-cli:masterfrom
johanrd:day_fix/template-no-block-params-for-html-elements

Conversation

@johanrd
Copy link
Copy Markdown
Contributor

@johanrd johanrd commented Apr 13, 2026

Summary

  • Adds svg-tags to the element allowlist alongside existing html-tags — so <circle as |x|> etc. are now flagged.
  • Documents accepted false negatives via new test cases: custom elements (<my-element as |x|>) and namespaced components (<NS.Foo as |x|>) aren't in the allowlists, so they're not flagged. Web-component namespace is open and can't be enumerated.
  • Minor comment cleanup in the rule.

Test plan

  • <div as |x|> / <section as |x|> / <ul as |x|> → flagged
  • <MyComponent as |x|> → valid (uppercase, not in allowlist)
  • <NS.Foo as |x|> → valid (dotted)
  • <my-element as |x|> → valid (accepted false negative — web component)
  • <div as |x|> where div is a local binding in GJS → valid (scope check)

@johanrd johanrd marked this pull request as ready for review April 13, 2026 16:10
@johanrd johanrd force-pushed the day_fix/template-no-block-params-for-html-elements branch from a6eaaf3 to f6dcffa Compare April 13, 2026 17:38
@johanrd johanrd changed the title Fix template-no-block-params-for-html-elements: align HTML-element detection Post-merge-review: Fix template-no-block-params-for-html-elements: align HTML-element detection Apr 13, 2026
Copy link
Copy Markdown
Contributor

@NullVoxPopuli NullVoxPopuli left a comment

Choose a reason for hiding this comment

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

I don't agree with the direction of this PR -- using a static list for html-tags is better because we don't actually have good hueristics for detecting if something is a component or not, since components can be defined as htmltags

Copy link
Copy Markdown
Contributor

@NullVoxPopuli NullVoxPopuli left a comment

Choose a reason for hiding this comment

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

Don't remove html-tags

@johanrd
Copy link
Copy Markdown
Contributor Author

johanrd commented Apr 14, 2026

@NullVoxPopuli extending html-tags could patch some gaps, yes. It would miss SVG elements (<path>) and MathML elements (<mfrac>) — but those are solvable by adding svg-tags or similar packages alongside.

But what about Web Components? Their naming convention is lowercase-with-hyphen (<my-widget>), and by definition there's no finite enumerable list of them — the namespace is open to anyone publishing a component library. So <my-widget as |x|>{{x}}</my-widget> is unambiguously a bug, but no combination of tag-list packages can ever catch it.

The inverse approach sidesteps this entirely because the set of things that are Glimmer component invocations is small and bounded (uppercase first char, ., @, this., local scope) — everything else is "not a component" by what the runtime actually does.

Ember-template-lint's own implementation of these rules already reached this conclusion — https://github.com/ember-template-lint/ember-template-lint/blob/main/lib/helpers/is-angle-bracket-component.js uses exactly this inverse detection with no html-tags allowlist anywhere. The conservative approach is to go with the ember-template-lint implementation for now, no?

@NullVoxPopuli
Copy link
Copy Markdown
Contributor

html-tags is not meant to have all tags, but is meant to give us confidence that the tags it does have are HTML -- there are separate packages for svg-tags and mathml-tags,.

web components

these cannot be known ahead of time, so we have to fallback to lower-case with hyphen check... however, do we care if we see something as a web-component?

if it's in scope, we know wheer it is. if it's not in scope, it's defined globally (html, svg, mathml, web-component), etc.

isAngleBracketComponent is flawed and only is possible of working in loosemode, we can onlyl suceeed through known lists and scope analysis.

@johanrd
Copy link
Copy Markdown
Contributor Author

johanrd commented Apr 14, 2026

Thanks, for the pushback. You are probably right. I can revert to lists + scope (and add svg-tags / mathml-tags alongside?).

The only tradeoff/regression (as i understand it) is then in HBS mode, where <foo as |x|> is a silent false negative under the html-list approach?

Mode Lists + scope Inverse heuristic
GTS caught via scope ✓ caught via lowercase check ✓
HBS missed (empty scope + not in any list) caught ✓

@NullVoxPopuli
Copy link
Copy Markdown
Contributor

hbs should have scope enabled for <foo as |x|> -- there should be no tradeoff to using scope

@johanrd
Copy link
Copy Markdown
Contributor Author

johanrd commented Apr 15, 2026

Investigated this through claude: The HBS parser seems to create an empty scope manager. From node_modules/ember-eslint-parser/src/parser/hbs-parser.js lines 70–81:

// Create an empty scope manager.
// For HBS, all locals are assumed to be defined at runtime,
// so we don't track variable references (no no-undef errors).
const scopeManager = eslintScope.analyze(
  {
    type: 'Program',
    body: [],  // ← empty — no bindings registered
    range: [0, code.length],
    loc: program.loc,
  },
  { range: true }
);

Block params from <foo as |x|> exist on GlimmerElementNode.blockParams in the AST, but they are not registered in the scope manager in HBS mode. context.getScope() returns a global scope with no child scopes and no template-defined variables.

In GTS/GJS mode the main parser uses buildGlimmerVisitors (via ember-estree's toTree) which does register block params in scope. HBS mode calls toTree(code, { templateOnly: true }) with no visitors, so scope stays empty.

Concretely, in HBS mode scope.references.some(ref => ref.identifier === node.parts[0]) is always false — the scope check in the current implementation is dead code for HBS files. The isHtmlElement heuristic is what carries the HBS case.

If scope support for block params is added to ember-eslint-parser/hbs in the future, the heuristic can be dropped. For now it's necessary unless hbs can live with the uncaught of course.

@NullVoxPopuli
Copy link
Copy Markdown
Contributor

claude, you do it

@johanrd
Copy link
Copy Markdown
Contributor Author

johanrd commented Apr 15, 2026

@NullVoxPopuli are you intending to

  1. fix ember-eslint-parser/hbs to populate block-param scope? (i will not touch that, i think)
  2. or explicitly accept HBS false negatives opposed to the implementation of ember-template-lint?

Which?

@NullVoxPopuli
Copy link
Copy Markdown
Contributor

I'm not intending anything -- you are / are using a bot, so the bot should fix the hbs parser to include scope <3 🎉.

Please PR to ember-eslint-parser

@johanrd
Copy link
Copy Markdown
Contributor Author

johanrd commented Apr 15, 2026

okay, thanks. (I, as a human, genuinely did not understand from your brevity)

See ember-tooling/ember-eslint-parser#189

Switched to html-tags + svg-tags + scope now. Custom elements and other non-listed lowercase tags are accepted false negatives. MathML skipped for now — mathml-tag-names@4 is ESM-only.

…h svg-tags

Adds svg-tags to the html-tags allowlist so <circle as |x|> etc. are
flagged alongside HTML elements. Documents accepted false negatives
(custom elements, namespaced components) via new test cases.
@johanrd johanrd force-pushed the day_fix/template-no-block-params-for-html-elements branch from 4faabdc to 6308771 Compare April 15, 2026 18:45
@johanrd johanrd changed the title Post-merge-review: Fix template-no-block-params-for-html-elements: align HTML-element detection Post-merge-review: extend allowlist with svg-tags on template-no-block-params-for-html-elements Apr 15, 2026
@NullVoxPopuli
Copy link
Copy Markdown
Contributor

We support only the ESM-only nodes. so... node 20.19+ and all other nodes that have require(esm) -- we can use ESM-only packages

…ames

Extends the allowlist with MathML tag names. NullVoxPopuli confirmed the
plugin can use ESM-only packages since the supported Node range (>=20.19)
has require(esm). Adds an invalid test for `<mfrac as |num|>` and an
invalid test for `<circle as |r|>` (SVG, via svg-tags).
@johanrd
Copy link
Copy Markdown
Contributor Author

johanrd commented Apr 15, 2026

okay, thanks, didn't know. added now.

@johanrd johanrd requested a review from NullVoxPopuli April 15, 2026 20:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants