Post-merge-review: extend allowlist with svg-tags on template-no-block-params-for-html-elements#2689
Conversation
a6eaaf3 to
f6dcffa
Compare
NullVoxPopuli
left a comment
There was a problem hiding this comment.
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
NullVoxPopuli
left a comment
There was a problem hiding this comment.
Don't remove html-tags
|
@NullVoxPopuli extending But what about Web Components? Their naming convention is lowercase-with-hyphen ( 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? |
|
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,.
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. |
|
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
|
|
hbs should have scope enabled for |
|
Investigated this through claude: The HBS parser seems to create an empty scope manager. From // 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 In GTS/GJS mode the main parser uses Concretely, in HBS mode If scope support for block params is added to |
|
claude, you do it |
|
@NullVoxPopuli are you intending to
Which? |
|
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 |
|
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.
4faabdc to
6308771
Compare
|
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).
|
okay, thanks, didn't know. added now. |
ember-tooling/ember-eslint-parser#189 populates block-param scope for <foo as |x|> in HBS mode, so the rule's scope check in lib/rules/template-no-block-params-for-html-elements.js no longer relies on an empty scope manager in .hbs files. No observable test change on master (all tests here use <template>...</template> / GJS mode where scope already worked), but makes the scope-based detection correct in HBS too.
…classification) Adds `lib/utils/is-native-element.js` — a shared util for the recurring "is this a native HTML/SVG/MathML element or a component?" question. Uses the authoritative `html-tags` + `svg-tags` + `mathml-tag-names` packages plus scope analysis, mirroring the pattern established by @NullVoxPopuli in ember-cli#2689 (template-no-block-params-for-html-elements). ## Why not heuristics An earlier draft of this PR used a lexical heuristic (PascalCase / `@` / `this.` / dot / `::`). That approach was rejected in the ember-cli#2689 discussion — a lowercase tag CAN be a component in GJS/GTS when its name is bound in scope (`const div = MyComponent; <template><div /></template>`), so heuristics produce both false positives (web components misclassified as native) and false negatives (scope-shadowed tags misclassified as native). Lists + scope handles both correctly. ## What the util does `isNativeElement(node, sourceCode)` returns true iff: - The tag name is in the HTML/SVG/MathML authoritative lists, AND - The tag name is NOT a scope-bound identifier. Returns false for components (PascalCase, dotted, @-prefixed, etc. — none of those tag-name shapes appear in the lists), custom elements (`<my-element>` — accepted false negative; web component namespace is open-ended), and scope-bound identifiers. ## Rules migrated Four rules now share the util: - `template-no-empty-headings` — fixes the `<Article><span>…</span></Article>` misclassification (PascalCase component containing text, previously treated as an empty heading via `.toLowerCase()` lookup). - `template-no-invalid-interactive` — fixes `<Article role="button">` / `<Article tabindex={{0}}>` false positives. - `template-no-arguments-for-html-elements` — previously carried inline copy of the lists+scope pattern (established in ember-cli#2689); now uses the shared util. - `template-no-block-params-for-html-elements` — same. This is the rule that introduced the pattern, now deduplicated. Net: 2 inline copies of the canonical pattern dropped; 2 buggy rules fixed; +173 -57 lines.
…classification) Adds `lib/utils/is-native-element.js` — a shared util for the recurring "is this a native HTML/SVG/MathML element or a component?" question. Uses the authoritative `html-tags` + `svg-tags` + `mathml-tag-names` packages plus scope analysis, mirroring the pattern established by @NullVoxPopuli in ember-cli#2689 (template-no-block-params-for-html-elements). ## Why not heuristics An earlier draft of this PR used a lexical heuristic (PascalCase / `@` / `this.` / dot / `::`). That approach was rejected in the ember-cli#2689 discussion — a lowercase tag CAN be a component in GJS/GTS when its name is bound in scope (`const div = MyComponent; <template><div /></template>`), so heuristics produce both false positives (web components misclassified as native) and false negatives (scope-shadowed tags misclassified as native). Lists + scope handles both correctly. ## What the util does `isNativeElement(node, sourceCode)` returns true iff: - The tag name is in the HTML/SVG/MathML authoritative lists, AND - The tag name is NOT a scope-bound identifier. Returns false for components (PascalCase, dotted, @-prefixed, etc. — none of those tag-name shapes appear in the lists), custom elements (`<my-element>` — accepted false negative; web component namespace is open-ended), and scope-bound identifiers. ## Rules migrated Four rules now share the util: - `template-no-empty-headings` — fixes the `<Article><span>…</span></Article>` misclassification (PascalCase component containing text, previously treated as an empty heading via `.toLowerCase()` lookup). - `template-no-invalid-interactive` — fixes `<Article role="button">` / `<Article tabindex={{0}}>` false positives. - `template-no-arguments-for-html-elements` — previously carried inline copy of the lists+scope pattern (established in ember-cli#2689); now uses the shared util. - `template-no-block-params-for-html-elements` — same. This is the rule that introduced the pattern, now deduplicated. Net: 2 inline copies of the canonical pattern dropped; 2 buggy rules fixed; +173 -57 lines.
…classification) Adds `lib/utils/is-native-element.js` — a shared util for the recurring "is this a native HTML/SVG/MathML element or a component?" question. Uses the authoritative `html-tags` + `svg-tags` + `mathml-tag-names` packages plus scope analysis, mirroring the pattern established by @NullVoxPopuli in ember-cli#2689 (template-no-block-params-for-html-elements). ## Why not heuristics An earlier draft of this PR used a lexical heuristic (PascalCase / `@` / `this.` / dot / `::`). That approach was rejected in the ember-cli#2689 discussion — a lowercase tag CAN be a component in GJS/GTS when its name is bound in scope (`const div = MyComponent; <template><div /></template>`), so heuristics produce both false positives (web components misclassified as native) and false negatives (scope-shadowed tags misclassified as native). Lists + scope handles both correctly. ## What the util does `isNativeElement(node, sourceCode)` returns true iff: - The tag name is in the HTML/SVG/MathML authoritative lists, AND - The tag name is NOT a scope-bound identifier. Returns false for components (PascalCase, dotted, @-prefixed, etc. — none of those tag-name shapes appear in the lists), custom elements (`<my-element>` — accepted false negative; web component namespace is open-ended), and scope-bound identifiers. ## Rules migrated Four rules now share the util: - `template-no-empty-headings` — fixes the `<Article><span>…</span></Article>` misclassification (PascalCase component containing text, previously treated as an empty heading via `.toLowerCase()` lookup). - `template-no-invalid-interactive` — fixes `<Article role="button">` / `<Article tabindex={{0}}>` false positives. - `template-no-arguments-for-html-elements` — previously carried inline copy of the lists+scope pattern (established in ember-cli#2689); now uses the shared util. - `template-no-block-params-for-html-elements` — same. This is the rule that introduced the pattern, now deduplicated. Net: 2 inline copies of the canonical pattern dropped; 2 buggy rules fixed; +173 -57 lines.
…classification) Adds `lib/utils/is-native-element.js` — a shared util for the recurring "is this a native HTML/SVG/MathML element or a component?" question. Uses the authoritative `html-tags` + `svg-tags` + `mathml-tag-names` packages plus scope analysis, mirroring the pattern established by @NullVoxPopuli in ember-cli#2689 (template-no-block-params-for-html-elements). ## Why not heuristics An earlier draft of this PR used a lexical heuristic (PascalCase / `@` / `this.` / dot / `::`). That approach was rejected in the ember-cli#2689 discussion — a lowercase tag CAN be a component in GJS/GTS when its name is bound in scope (`const div = MyComponent; <template><div /></template>`), so heuristics produce both false positives (web components misclassified as native) and false negatives (scope-shadowed tags misclassified as native). Lists + scope handles both correctly. ## What the util does `isNativeElement(node, sourceCode)` returns true iff: - The tag name is in the HTML/SVG/MathML authoritative lists, AND - The tag name is NOT a scope-bound identifier. Returns false for components (PascalCase, dotted, @-prefixed, etc. — none of those tag-name shapes appear in the lists), custom elements (`<my-element>` — accepted false negative; web component namespace is open-ended), and scope-bound identifiers. ## Rules migrated Four rules now share the util: - `template-no-empty-headings` — fixes the `<Article><span>…</span></Article>` misclassification (PascalCase component containing text, previously treated as an empty heading via `.toLowerCase()` lookup). - `template-no-invalid-interactive` — fixes `<Article role="button">` / `<Article tabindex={{0}}>` false positives. - `template-no-arguments-for-html-elements` — previously carried inline copy of the lists+scope pattern (established in ember-cli#2689); now uses the shared util. - `template-no-block-params-for-html-elements` — same. This is the rule that introduced the pattern, now deduplicated. Net: 2 inline copies of the canonical pattern dropped; 2 buggy rules fixed; +173 -57 lines.
…+ scope) Replace the inline PascalCase-regex isComponentInvocation heuristic with the lists + scope pattern established by @NullVoxPopuli in ember-cli#2689 and canonicalised by ember-cli#2724. The new lib/utils/is-native-element.js mirrors ember-cli#2724's util byte-for-byte so the two PRs can land in either order without conflict — whichever lands first introduces the file; the other rebases to a no-op on it. Heuristic approaches (PascalCase regex, `tag.includes('.')`, etc.) were explicitly rejected in ember-cli#2689 because lowercase tags CAN be components in GJS/GTS when the name is shadowed by a scope binding (`const div = MyComponent; <div />`). Scope analysis catches this; regex heuristics don't. Call site now uses `!isNativeElement(node, sourceCode)` directly to match the canonical usage pattern in ember-cli#2724's migrated rules.
…+ scope) Remove the inline PascalCase-regex isComponentInvocation heuristic in favor of the lists + scope pattern from ember-cli#2689 / ember-cli#2724. The new lib/utils/is-native-element.js mirrors ember-cli#2724's util byte-for-byte so the two PRs can land in either order without conflict. evaluateChild / evaluateChildren now thread sourceCode through the recursion so the scope check has access to the enclosing template's bindings. Heuristic approaches were explicitly rejected in ember-cli#2689 because lowercase tags CAN be components when shadowed by scope bindings. Treating custom elements as opaque (the same as components) is a behavior improvement — matches ember-cli#2724's convention.
…classification) Adds `lib/utils/is-native-element.js` — a shared util for the recurring "is this a native HTML/SVG/MathML element or a component?" question. Uses the authoritative `html-tags` + `svg-tags` + `mathml-tag-names` packages plus scope analysis, mirroring the pattern established by @NullVoxPopuli in ember-cli#2689 (template-no-block-params-for-html-elements). An earlier draft of this PR used a lexical heuristic (PascalCase / `@` / `this.` / dot / `::`). That approach was rejected in the ember-cli#2689 discussion — a lowercase tag CAN be a component in GJS/GTS when its name is bound in scope (`const div = MyComponent; <template><div /></template>`), so heuristics produce both false positives (web components misclassified as native) and false negatives (scope-shadowed tags misclassified as native). Lists + scope handles both correctly. `isNativeElement(node, sourceCode)` returns true iff: - The tag name is in the HTML/SVG/MathML authoritative lists, AND - The tag name is NOT a scope-bound identifier. Returns false for components (PascalCase, dotted, @-prefixed, etc. — none of those tag-name shapes appear in the lists), custom elements (`<my-element>` — accepted false negative; web component namespace is open-ended), and scope-bound identifiers. Four rules now share the util: - `template-no-empty-headings` — fixes the `<Article><span>…</span></Article>` misclassification (PascalCase component containing text, previously treated as an empty heading via `.toLowerCase()` lookup). - `template-no-invalid-interactive` — fixes `<Article role="button">` / `<Article tabindex={{0}}>` false positives. - `template-no-arguments-for-html-elements` — previously carried inline copy of the lists+scope pattern (established in ember-cli#2689); now uses the shared util. - `template-no-block-params-for-html-elements` — same. This is the rule that introduced the pattern, now deduplicated. Net: 2 inline copies of the canonical pattern dropped; 2 buggy rules fixed; +173 -57 lines.
…+ scope) Remove the inline PascalCase-regex isComponentInvocation heuristic in favor of the lists + scope pattern from ember-cli#2689 / ember-cli#2724. The new lib/utils/is-native-element.js mirrors ember-cli#2724's util byte-for-byte so the two PRs can land in either order without conflict. evaluateChild / evaluateChildren now thread sourceCode through the recursion so the scope check has access to the enclosing template's bindings. Heuristic approaches were explicitly rejected in ember-cli#2689 because lowercase tags CAN be components when shadowed by scope bindings. Treating custom elements as opaque (the same as components) is a behavior improvement — matches ember-cli#2724's convention.
…+ scope) Replace the inline PascalCase-regex isComponentInvocation heuristic with the lists + scope pattern established by @NullVoxPopuli in ember-cli#2689 and canonicalised by ember-cli#2724. The new lib/utils/is-native-element.js mirrors without conflict — whichever lands first introduces the file; the other rebases to a no-op on it. Heuristic approaches (PascalCase regex, `tag.includes('.')`, etc.) were explicitly rejected in ember-cli#2689 because lowercase tags CAN be components in GJS/GTS when the name is shadowed by a scope binding (`const div = MyComponent; <div />`). Scope analysis catches this; regex heuristics don't. Call site now uses `!isNativeElement(node, sourceCode)` directly to match the canonical usage pattern in ember-cli#2724's migrated rules.
…+ scope) Replace the inline PascalCase-regex isComponentInvocation heuristic with the lists + scope pattern established by @NullVoxPopuli in ember-cli#2689 and canonicalised by ember-cli#2724. The new lib/utils/is-native-element.js mirrors without conflict — whichever lands first introduces the file; the other rebases to a no-op on it. Heuristic approaches (PascalCase regex, `tag.includes('.')`, etc.) were explicitly rejected in ember-cli#2689 because lowercase tags CAN be components in GJS/GTS when the name is shadowed by a scope binding (`const div = MyComponent; <div />`). Scope analysis catches this; regex heuristics don't. Call site now uses `!isNativeElement(node, sourceCode)` directly to match the canonical usage pattern in ember-cli#2724's migrated rules.
…+ scope) Remove the inline PascalCase-regex isComponentInvocation heuristic in favor of the lists + scope pattern from ember-cli#2689 / ember-cli#2724. The new lib/utils/is-native-element.js mirrors ember-cli#2724's util byte-for-byte so the two PRs can land in either order without conflict. evaluateChild / evaluateChildren now thread sourceCode through the recursion so the scope check has access to the enclosing template's bindings. Heuristic approaches were explicitly rejected in ember-cli#2689 because lowercase tags CAN be components when shadowed by scope bindings. Treating custom elements as opaque (the same as components) is a behavior improvement — matches ember-cli#2724's convention.
Summary
svg-tagsto the element allowlist alongside existinghtml-tags— so<circle as |x|>etc. are now flagged.<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.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|>wheredivis a local binding in GJS → valid (scope check)