Skip to content

Extract rule: template-no-yield-only#2596

Merged
NullVoxPopuli merged 2 commits intoember-cli:masterfrom
NullVoxPopuli:nvp/template-lint-extract-rule-template-no-yield-only
Mar 20, 2026
Merged

Extract rule: template-no-yield-only#2596
NullVoxPopuli merged 2 commits intoember-cli:masterfrom
NullVoxPopuli:nvp/template-lint-extract-rule-template-no-yield-only

Conversation

@NullVoxPopuli
Copy link
Contributor

Split from #2371.

Copy link

@NullVoxPopuli-ai-agent NullVoxPopuli-ai-agent left a comment

Choose a reason for hiding this comment

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

Review: template-no-yield-only (PR #2596)

Comparison with ember-template-lint no-yield-only.js

General correctness:

  1. Logic difference in what counts as "empty": The original's isEmptyNode considers MustacheCommentStatement, CommentStatement, and whitespace-only TextNode as empty. The PR's IGNORABLE_TYPES set only includes GlimmerTextNode and GlimmerMustacheCommentStatement, but the isMeaningfulContent function also handles GlimmerTextNode specially (checking if trimmed content is non-empty). Missing: GlimmerCommentStatement (HTML comments like <!-- ... -->) is not in IGNORABLE_TYPES, so an HTML comment + yield would NOT trigger the rule, whereas in the original it would. Consider adding 'GlimmerCommentStatement' to IGNORABLE_TYPES.

  2. isYieldOnly vs isBareYield: The original checks node.params.length === 0 but does not check node.hash. The PR additionally checks !node.hash || !node.hash.pairs || node.hash.pairs.length === 0. This is actually stricter — {{yield to="inverse"}} has zero params but has hash pairs, and the original would consider it a "yield only" while the PR would not. Looking at the test cases, {{yield (hash someProp=someValue)}} is valid in both, which has params. The PR's extra hash check seems correct/desirable since {{yield to="inverse"}} is not really a "bare yield".

    Wait — actually re-reading the original: it checks node.params.length === 0 which would be true for {{yield to="inverse"}} (hash pairs are not params). So the original would flag {{yield to="inverse"}} as yield-only, which seems like a bug in the original. The PR's stricter check that also requires no hash pairs is actually an improvement.

  3. GlimmerTemplate visitor: The PR handles both gjs (where body[0] is a GlimmerElementNode for <template>) and hbs (where body directly contains nodes). This is a reasonable approach though the heuristic of checking firstChild.type === 'GlimmerElementNode' could theoretically be fragile if the parser representation changes. Consider adding a comment explaining this distinction.

Scope analysis (gjs/gts):

The rule checks node.path.original === 'yield'. yield is a Glimmer keyword and cannot be shadowed. No scope analysis needed.

Overall: Good migration with one minor gap — consider adding GlimmerCommentStatement to IGNORABLE_TYPES to match the original's treatment of HTML comments as empty content.

🤖 Automated review comparing with ember-template-lint source

@NullVoxPopuli NullVoxPopuli force-pushed the nvp/template-lint-extract-rule-template-no-yield-only branch from 805a524 to 6bc2977 Compare March 20, 2026 22:03
@NullVoxPopuli NullVoxPopuli merged commit 62e11ee into ember-cli:master Mar 20, 2026
9 checks passed
@NullVoxPopuli NullVoxPopuli deleted the nvp/template-lint-extract-rule-template-no-yield-only branch March 20, 2026 22:22
@github-actions github-actions bot mentioned this pull request Mar 20, 2026
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