Extract rule: template-require-each-key#2602
Extract rule: template-require-each-key#2602NullVoxPopuli wants to merge 1 commit intoember-cli:masterfrom
Conversation
NullVoxPopuli-ai-agent
left a comment
There was a problem hiding this comment.
Review: template-require-each-key
General Correctness
1. Error message differs from original. The original uses "{{#each}} helper requires a valid key value to avoid performance issues" while the PR uses "each block should have a key attribute for better rendering performance." This is a minor stylistic difference, but worth noting for consistency.
2. The isValidKey function has slightly different semantics than the original. The original rule checks keyPair.value.value (the literal string value) and considers a falsy keyValue as invalid. The PR's isValidKey function returns true for non-GlimmerStringLiteral types (treating dynamic values as OK), which matches the original intent. However, the PR adds an explicit empty-string check (value.trim() === '') and an explicit invalid-@-key check that the original handles more concisely via isValidKey = isSpecialKey ? SPECIAL_KEY_VALUES.has(keyValue) : keyValue. The behavior should be equivalent, but the PR's approach is more explicit, which is fine.
3. Missing @identity in the VALID_AT_KEYS set? Actually no, it's there. Both the original and the PR correctly handle @identity and @index. Good.
4. Tests look solid. Both gjs and hbs modes are covered. Edge cases for empty key, invalid @-key, and missing key are all tested.
Scope Analysis (gjs/gts)
This rule matches node.path.original === 'each' to detect {{#each}} blocks. In Glimmer templates, each is a built-in keyword/helper. In gjs/gts with strict mode, each is imported from @ember/helper or similar. Technically, a user could shadow each with a local variable, but in practice {{#each}} as a block statement is a language-level construct that the parser recognizes specially. No scope analysis needed -- each as a block statement keyword cannot be meaningfully shadowed in a way that would cause false positives.
Summary
Looks good overall. The implementation faithfully captures the original rule's behavior with appropriate adaptations for ESLint.
🤖 Automated review comparing with ember-template-lint source
Split from #2371.