Closed
Conversation
This reverts commit 7a06d44.
The rule flagged yield-only templates by asserting `templateNodes.length === 1 && isYieldOnly(templateNodes[0])`. That works only while the parser strips template comments out of the body, which ember-estree 0.4.2 happened to do. Upstream ember-estree 0.4.3 (#31) began keeping MustacheCommentStatement nodes in the body, so a template like <template>{{! some comment }}{{yield}}</template> now yields a body of length 2 and silently stops being flagged — which is also why PR ember-cli#2735 (upgrade to ember-eslint-parser 0.11) fails this rule's CI. Ignoring comment and whitespace-only text nodes before the length check aligns with: - upstream ember-template-lint's `no-yield-only.js` (unchanged there since comments-in-body is the native Glimmer AST shape anyway) - the sibling rule `template-no-bare-yield.js`, which already uses the same `isEmptyNode` filter The fix is a no-op on the currently-pinned [email protected] and fixes the rule under any parser version that preserves comment nodes in the template body. The rule also now correctly catches yield-only templates that contain HTML comments (`<!-- x -->{{yield}}`), which it was silently missing before.
🏎️ Benchmark Comparison
Full mitata output |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Cowritten by claude:
Summary
template-no-yield-onlyflags yield-only templates viatemplateNodes.length === 1 && isYieldOnly(templateNodes[0]). That holds only while the parser strips template comments out of the body, which[email protected]happened to do.[email protected](NullVoxPopuli/ember-estree#31) started keepingMustacheCommentStatementnodes in the body (the native Glimmer AST shape). Under that parser a template like<template>{{! x }}{{yield}}</template>has a body of length 2 and is silently no longer flagged. This is the rule failure that blocks Update ember-eslint-parser to 0.11 ember-cli/eslint-plugin-ember#2735's CI.isEmptyNodehelper that upstreamember-template-lintand the sibling ruletemplate-no-bare-yield.jsalready use.Why fix it here rather than in the parser / estree
[email protected]'s change is the correct AST: it matches the pure-hbs parser (ember-eslint-parser/hbs) and the native Glimmer AST, both of which have always kept comment nodes in the template body. Re-hiding those nodes at the parser layer would re-introduce the bug that ember-cli#2733 guards against ({{! eslint-disable-* }}directives would again be invisible to tooling that needs to see them). The semantic decision "should a comment-plus-yield template count as yield-only?" belongs to the rule.Notes
template-no-bare-yield.jsandtemplate-no-yield-only.jsare near-duplicate ports ofember-template-lint/lib/rules/no-yield-only.js; only the former had kept pace with upstream's empty-node filter. This closes that drift.[email protected], so this is safe to land independently of Update ember-eslint-parser to 0.11 ember-cli/eslint-plugin-ember#2735 and paves the way for that PR's CI to go green once rebased.<!-- html -->{{yield}}was not being flagged as yield-only; now it is.Test plan
pnpm test tests/lib/rules/template-no-yield-only.js— 22 passed (18 original + 4 new assertions:{{!-- long --}}{{yield}}and<!-- x -->{{yield}}against both the gjs and hbs parsers)npx prettier --check+npx eslintclean on both modified files