Skip to content

Post-merge-review: Fix template-require-iframe-title: split messageIds; report each duplicate with index #2684

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
johanrd wants to merge 1 commit into
base: master
Choose a base branch
from
+126 −17
Open

Post-merge-review: Fix template-require-iframe-title: split messageIds; report each duplicate with index #2684

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 42 additions & 8 deletions lib/rules/template-require-iframe-title.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,14 @@ module.exports = {
},
schema: [],
messages: {
// Split from a single `missingTitle` into four messageIds aligned with
// upstream ember-template-lint, providing richer diagnostic detail.
missingTitle: '<iframe> elements must have a unique title property.',
emptyTitle: '<iframe> elements must have a unique title property.',
dynamicFalseTitle: '<iframe> elements must have a unique title property.',
duplicateTitleFirst: 'This title is not unique. #{{index}}',
duplicateTitleOther:
'<iframe> elements must have a unique title property. Value title="{{title}}" already used for different iframe. #{{index}}',
},
originallyFrom: {
name: 'ember-template-lint',
Expand All @@ -20,7 +27,12 @@ module.exports = {
},
},
create(context) {
// Each entry: { value, node, index }
// - value: trimmed title string
// - node: original element node for the first occurrence
// - index: duplicate-group index (1-based), assigned lazily on collision
const knownTitles = [];
let nextDuplicateIndex = 1;

return {
GlimmerElementNode(node) {
Expand All @@ -47,22 +59,44 @@ module.exports = {
case 'GlimmerTextNode': {
const value = titleAttr.value.chars.trim();
if (value.length === 0) {
context.report({ node, messageId: 'missingTitle' });
context.report({ node, messageId: 'emptyTitle' });
} else {
// Check for duplicate titles
const existingIdx = knownTitles.findIndex(([val]) => val === value);
if (existingIdx === -1) {
knownTitles.push([value, node]);
// Check for duplicate titles. Upstream reports BOTH the
// first and the current occurrence of a duplicated title
// on every collision, sharing a `#N` index so users can
// correlate them. For three or more duplicates the first
// occurrence is therefore re-reported once per collision.
const existing = knownTitles.find((entry) => entry.value === value);
if (existing) {
if (existing.index === null) {
existing.index = nextDuplicateIndex++;
}
const index = existing.index;

// Report on the first occurrence on every collision,
// matching upstream ember-template-lint behavior.
context.report({
node: existing.node,
messageId: 'duplicateTitleFirst',
data: { index: String(index) },
});

// Report on the current (duplicate) occurrence.
context.report({
node,
messageId: 'duplicateTitleOther',
data: { title: titleAttr.value.chars, index: String(index) },
});
} else {
context.report({ node, messageId: 'missingTitle' });
knownTitles.push({ value, node, index: null });
}
}
break;
}
case 'GlimmerMustacheStatement': {
// title={{false}} → BooleanLiteral false is invalid
if (titleAttr.value.path?.type === 'GlimmerBooleanLiteral') {
context.report({ node, messageId: 'missingTitle' });
context.report({ node, messageId: 'dynamicFalseTitle' });
}
break;
}
Expand All @@ -74,7 +108,7 @@ module.exports = {
parts[0].type === 'GlimmerMustacheStatement' &&
parts[0].path?.type === 'GlimmerBooleanLiteral'
) {
context.report({ node, messageId: 'missingTitle' });
context.report({ node, messageId: 'dynamicFalseTitle' });
}
break;
}
Expand Down
93 changes: 84 additions & 9 deletions tests/lib/rules/template-require-iframe-title.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,18 +28,61 @@ ruleTester.run('template-require-iframe-title', rule, {
{
code: '<template><iframe title=""></iframe></template>',
output: null,
errors: [{ messageId: 'missingTitle' }],
errors: [{ messageId: 'emptyTitle' }],
},

{
// Upstream reports BOTH occurrences with a shared `#N` index.
code: '<template><iframe title="foo" /><iframe title="foo" /></template>',
output: null,
errors: [{ messageId: 'missingTitle' }],
errors: [
{ message: 'This title is not unique. #1' },
{
message:
'<iframe> elements must have a unique title property. Value title="foo" already used for different iframe. #1',
},
],
},
{
// Three duplicates → upstream re-reports the first occurrence on every
// collision, so iframe #1 is flagged twice (once per later collision)
// and iframes #2 and #3 are each flagged once. ESLint sorts by source
// location, so the two first-occurrence reports (same location) come
// before the two later occurrences.
code: '<template><iframe title="foo" /><iframe title="foo" /><iframe title="foo" /></template>',
output: null,
errors: [
{ message: 'This title is not unique. #1' },
{ message: 'This title is not unique. #1' },
{
message:
'<iframe> elements must have a unique title property. Value title="foo" already used for different iframe. #1',
},
{
message:
'<iframe> elements must have a unique title property. Value title="foo" already used for different iframe. #1',
},
],
},
{
// Two distinct duplicate groups → 4 reports, indices #1 and #2.
// ESLint sorts errors by source location; the two "first-occurrence"
// reports attach to the first two iframes and so precede the two
// "other-occurrence" reports attached to the later iframes.
code: '<template><iframe title="foo" /><iframe title="boo" /><iframe title="foo" /><iframe title="boo" /></template>',
output: null,
errors: [{ messageId: 'missingTitle' }, { messageId: 'missingTitle' }],
errors: [
{ message: 'This title is not unique. #1' },
{ message: 'This title is not unique. #2' },
{
message:
'<iframe> elements must have a unique title property. Value title="foo" already used for different iframe. #1',
},
{
message:
'<iframe> elements must have a unique title property. Value title="boo" already used for different iframe. #2',
},
],
},
{
code: '<template><iframe src="12" /></template>',
Expand All @@ -49,17 +92,17 @@ ruleTester.run('template-require-iframe-title', rule, {
{
code: '<template><iframe src="12" title={{false}} /></template>',
output: null,
errors: [{ messageId: 'missingTitle' }],
errors: [{ messageId: 'dynamicFalseTitle' }],
},
{
code: '<template><iframe src="12" title="{{false}}" /></template>',
output: null,
errors: [{ messageId: 'missingTitle' }],
errors: [{ messageId: 'dynamicFalseTitle' }],
},
{
code: '<template><iframe src="12" title="" /></template>',
output: null,
errors: [{ messageId: 'missingTitle' }],
errors: [{ messageId: 'emptyTitle' }],
},
],
});
Expand All @@ -84,14 +127,46 @@ hbsRuleTester.run('template-require-iframe-title', rule, {
{
code: '<iframe title="foo" /><iframe title="foo" />',
output: null,
errors: [{ message: '<iframe> elements must have a unique title property.' }],
errors: [
{ message: 'This title is not unique. #1' },
{
message:
'<iframe> elements must have a unique title property. Value title="foo" already used for different iframe. #1',
},
],
},
{
// Three duplicates: upstream re-reports the first occurrence on every
// collision, so iframe #1 is flagged twice and each later iframe once.
code: '<iframe title="foo" /><iframe title="foo" /><iframe title="foo" />',
output: null,
errors: [
{ message: 'This title is not unique. #1' },
{ message: 'This title is not unique. #1' },
{
message:
'<iframe> elements must have a unique title property. Value title="foo" already used for different iframe. #1',
},
{
message:
'<iframe> elements must have a unique title property. Value title="foo" already used for different iframe. #1',
},
],
},
{
code: '<iframe title="foo" /><iframe title="boo" /><iframe title="foo" /><iframe title="boo" />',
output: null,
errors: [
{ message: '<iframe> elements must have a unique title property.' },
{ message: '<iframe> elements must have a unique title property.' },
{ message: 'This title is not unique. #1' },
{ message: 'This title is not unique. #2' },
{
message:
'<iframe> elements must have a unique title property. Value title="foo" already used for different iframe. #1',
},
{
message:
'<iframe> elements must have a unique title property. Value title="boo" already used for different iframe. #2',
},
],
},
{
Expand Down
Loading