Skip to content
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

Jump to bottom

[Fix] no-invalid-html-attribute: allow 'shortcut icon' on link #3174

Merged
merged 2 commits into from Jan 17, 2022
Merged

[Fix] no-invalid-html-attribute: allow 'shortcut icon' on link #3174

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4c3d00f
[Tests] `no-invalid-html-attribute`: add some more tests
ljharb Jan 17, 2022
e0af60b
[Fix] `no-invalid-html-attribute`: allow 'shortcut icon' on `link`
Primajin Jan 11, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Expand Up @@ -10,11 +10,13 @@ This change log adheres to standards from [Keep a CHANGELOG](https://keepachange

### Fixed
* [`prop-types`], `propTypes`: add support for exported type inference ([#3163][] @vedadeepta)
* [`no-invalid-html-attribute`]: allow 'shortcut icon' on `link` ([#3174][] @Primajin)

### Changed
* [readme] change [`jsx-runtime`] link from branch to sha ([#3160][] @tatsushitoji)
* [Docs] HTTP => HTTPS ([#3133][] @Schweinepriester)

[#3174]: https://github.com/yannickcr/eslint-plugin-react/pull/3174
[#3163]: https://github.com/yannickcr/eslint-plugin-react/pull/3163
[#3160]: https://github.com/yannickcr/eslint-plugin-react/pull/3160
[#3133]: https://github.com/yannickcr/eslint-plugin-react/pull/3133
Expand Down
64 changes: 57 additions & 7 deletions lib/rules/no-invalid-html-attribute.js
View file
Open in desktop
Expand Up @@ -39,10 +39,16 @@ const rel = new Map([
['prerender', new Set(['link'])],
['prev', new Set(['link', 'area', 'a', 'form'])],
['search', new Set(['link', 'area', 'a', 'form'])],
['shortcut', new Set(['link'])], // generally allowed but needs pair with "icon"
['shortcut\u0020icon', new Set(['link'])],
['stylesheet', new Set(['link'])],
['tag', new Set(['area', 'a'])],
]);

const pairs = new Map([
['shortcut', new Set(['icon'])],
]);

/**
* Map between attributes and a mapping between valid values and a set of tags they are valid on
* @type {Map<string, Map<string, Set<string>>>}
Expand All @@ -51,6 +57,14 @@ const VALID_VALUES = new Map([
['rel', rel],
]);

/**
* Map between attributes and a mapping between pair-values and a set of values they are valid with
* @type {Map<string, Map<string, Set<string>>>}
*/
const VALID_PAIR_VALUES = new Map([
['rel', pairs],
]);

/**
* The set of all possible HTML elements. Used for skipping custom types
* @type {Set<string>}
Expand Down Expand Up @@ -216,6 +230,8 @@ const messages = {
noMethod: 'The ”{{attributeName}}“ attribute cannot be a method.',
onlyMeaningfulFor: 'The ”{{attributeName}}“ attribute only has meaning on the tags: {{tagNames}}',
emptyIsMeaningless: 'An empty “{{attributeName}}” attribute is meaningless.',
notAlone: '“{{reportingValue}}” must be directly followed by “{{missingValue}}”.',
notPaired: '“{{reportingValue}}” can not be directly followed by “{{secondValue}}” without “{{missingValue}}”.',
};

function splitIntoRangedParts(node, regex) {
Expand Down Expand Up @@ -256,10 +272,10 @@ function checkLiteralValueNode(context, attributeName, node, parentNode, parentN
return;
}

const parts = splitIntoRangedParts(node, /([^\s]+)/g);
for (const part of parts) {
const allowedTags = VALID_VALUES.get(attributeName).get(part.value);
const reportingValue = part.reportingValue;
const singleAttributeParts = splitIntoRangedParts(node, /(\S+)/g);
for (const singlePart of singleAttributeParts) {
const allowedTags = VALID_VALUES.get(attributeName).get(singlePart.value);
const reportingValue = singlePart.reportingValue;
if (!allowedTags) {
report(context, messages.neverValid, 'neverValid', {
node,
Expand All @@ -268,7 +284,7 @@ function checkLiteralValueNode(context, attributeName, node, parentNode, parentN
reportingValue,
},
fix(fixer) {
return fixer.removeRange(part.range);
return fixer.removeRange(singlePart.range);
},
});
} else if (!allowedTags.has(parentNodeName)) {
Expand All @@ -280,22 +296,56 @@ function checkLiteralValueNode(context, attributeName, node, parentNode, parentN
elementName: parentNodeName,
},
fix(fixer) {
return fixer.removeRange(part.range);
return fixer.removeRange(singlePart.range);
},
});
}
}

const allowedPairsForAttribute = VALID_PAIR_VALUES.get(attributeName);
if (allowedPairsForAttribute) {
const pairAttributeParts = splitIntoRangedParts(node, /(?=(\b\S+\s*\S+))/g);
for (const pairPart of pairAttributeParts) {
for (const [pairing, siblings] of allowedPairsForAttribute) {
const attributes = pairPart.reportingValue.split('\u0020');
const [firstValue, secondValue] = attributes;
if (firstValue === pairing) {
const lastValue = attributes[attributes.length - 1]; // in case of multiple white spaces
if (!siblings.has(lastValue)) {
const message = secondValue ? messages.notPaired : messages.notAlone;
const messageId = secondValue ? 'notPaired' : 'notAlone';
report(context, message, messageId, {
node,
data: {
reportingValue: firstValue,
secondValue,
missingValue: [...siblings].join(', '),
},
});
}
}
}
}
}

const whitespaceParts = splitIntoRangedParts(node, /(\s+)/g);
for (const whitespacePart of whitespaceParts) {
if (whitespacePart.value !== ' ' || whitespacePart.range[0] === (node.range[0] + 1) || whitespacePart.range[1] === (node.range[1] - 1)) {
if (whitespacePart.range[0] === (node.range[0] + 1) || whitespacePart.range[1] === (node.range[1] - 1)) {
report(context, messages.spaceDelimited, 'spaceDelimited', {
node,
data: { attributeName },
fix(fixer) {
return fixer.removeRange(whitespacePart.range);
},
});
} else if (whitespacePart.value !== '\u0020') {
report(context, messages.spaceDelimited, 'spaceDelimited', {
node,
data: { attributeName },
fix(fixer) {
return fixer.replaceTextRange(whitespacePart.range, '\u0020');
},
});
}
}
}
Expand Down
167 changes: 167 additions & 0 deletions tests/lib/rules/no-invalid-html-attribute.js
View file
Open in desktop
Expand Up @@ -130,6 +130,9 @@ ruleTester.run('no-invalid-html-attribute', rule, {
{ code: '<link rel="icon"></link>' },
{ code: 'React.createElement("link", { rel: "icon" })' },
{ code: 'React.createElement("link", { rel: ["icon"] })' },
{ code: '<link rel="shortcut icon"></link>' },
{ code: 'React.createElement("link", { rel: "shortcut icon" })' },
{ code: 'React.createElement("link", { rel: ["shortcut icon"] })' },
ljharb marked this conversation as resolved.
Show resolved Hide resolved
{ code: '<link rel="license"></link>' },
{ code: 'React.createElement("link", { rel: "license" })' },
{ code: 'React.createElement("link", { rel: ["license"] })' },
Expand Down Expand Up @@ -231,6 +234,83 @@ ruleTester.run('no-invalid-html-attribute', rule, {
},
],
invalid: [
{
code: '<a rel="alternatex"></a>',
output: '<a rel=""></a>',
errors: [
{
messageId: 'neverValid',
},
],
},
{
code: 'React.createElement("a", { rel: "alternatex" })',
output: 'React.createElement("a", { rel: "alternatex" })',
errors: [
{
messageId: 'neverValid',
},
],
},
{
code: 'React.createElement("a", { rel: ["alternatex"] })',
output: 'React.createElement("a", { rel: ["alternatex"] })',
errors: [
{
messageId: 'neverValid',
},
],
},
{
code: '<a rel="alternatex alternate"></a>',
output: '<a rel=" alternate"></a>',
errors: [
{
messageId: 'neverValid',
},
],
},
{
code: 'React.createElement("a", { rel: "alternatex alternate" })',
errors: [
{
messageId: 'neverValid',
},
],
},
{
code: 'React.createElement("a", { rel: ["alternatex alternate"] })',
errors: [
{
messageId: 'neverValid',
},
],
},
{
code: '<a rel="alternate alternatex"></a>',
output: '<a rel="alternate "></a>',
errors: [
{
messageId: 'neverValid',
},
],
},
{
code: 'React.createElement("a", { rel: "alternate alternatex" })',
errors: [
{
messageId: 'neverValid',
},
],
},
{
code: 'React.createElement("a", { rel: ["alternate alternatex"] })',
errors: [
{
messageId: 'neverValid',
},
],
},
{
code: '<html rel></html>',
output: '<html ></html>',
Expand Down Expand Up @@ -373,6 +453,26 @@ ruleTester.run('no-invalid-html-attribute', rule, {
},
],
},
{
code: '<a rel="noreferrer noopener"></a>',
output: '<a rel="noreferrer noopener"></a>',
errors: [
{
messageId: 'spaceDelimited',
data: { attributeName: 'rel' },
},
],
},
{
code: '<a rel="noreferrer\xa0\xa0noopener"></a>',
output: '<a rel="noreferrer noopener"></a>',
errors: [
{
messageId: 'spaceDelimited',
data: { attributeName: 'rel' },
},
],
},
{
code: '<a rel={"noreferrer noopener foobar"}></a>',
output: '<a rel={"noreferrer noopener "}></a>',
Expand Down Expand Up @@ -537,6 +637,73 @@ ruleTester.run('no-invalid-html-attribute', rule, {
},
],
},
{
code: '<link rel="shortcut"></link>',
errors: [
{
messageId: 'notAlone',
data: {
reportingValue: 'shortcut',
missingValue: 'icon',
},
},
],
},
{
code: '<link rel="shortcut foo"></link>',
output: '<link rel="shortcut "></link>',
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
output: '<link rel="shortcut "></link>',
output: '<link rel="shortcut"></link>',

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm but it actually does that - it's the opposite of

      code: '<a rel={"batgo noopener"}></a>',
      output: '<a rel={" noopener"}></a>',

Copy link
Contributor Author

Choose a reason for hiding this comment

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

 1) no-invalid-html-attribute
       invalid
         <link rel="shortcut foo"></link>:

      AssertionError [ERR_ASSERTION]: Output is incorrect.
      + expected - actual

      -<link rel="shortcut "></link>
      +<link rel="shortcut"></link>

The test fails when I remove it

This comment was marked as outdated.

Sign in to view

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh no, we shouldn't remove it - it is the testcase for the notPaired branch (in contrast to notAlone)

Copy link
Member

Choose a reason for hiding this comment

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

I'm basically saying that the autofix output should never have leading or trailing spaces.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that also confused me, but apparently that's "how things used to be" in master as well:

https://github.com/yannickcr/eslint-plugin-react/blob/0d8946212a1ba560b12e2e01807033e19dc774f7/tests/lib/rules/no-invalid-html-attribute.js#L355

https://github.com/yannickcr/eslint-plugin-react/blob/0d8946212a1ba560b12e2e01807033e19dc774f7/tests/lib/rules/no-invalid-html-attribute.js#L378

https://github.com/yannickcr/eslint-plugin-react/blob/0d8946212a1ba560b12e2e01807033e19dc774f7/tests/lib/rules/no-invalid-html-attribute.js#L403

https://github.com/yannickcr/eslint-plugin-react/blob/0d8946212a1ba560b12e2e01807033e19dc774f7/tests/lib/rules/no-invalid-html-attribute.js#L416

https://github.com/yannickcr/eslint-plugin-react/blob/0d8946212a1ba560b12e2e01807033e19dc774f7/tests/lib/rules/no-invalid-html-attribute.js#L477

Apparently it only runs one cycle when fixing and we would need to find a way to kind of while loop so long until it can't fix any more issues - but that sounds like a major refactor to me 🤔

Copy link
Member

Choose a reason for hiding this comment

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

fair point. altho it seems like adding .trim() to the attribute contents wouldn't be that difficult?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea! But where would I run this, I think I haven't fully grasped when/how fix runs. Would this be part of fix? Or would we just always in the end after all checks run another trim as it's never good/valid to keep it?

Copy link
Member

Choose a reason for hiding this comment

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

Adding a single fixer on program exit that trimmed any attribute strings that had been previously autofixed (we’d have to track that manually) would be good. I think that should wait for a followup PR, tho, because it was a pre-existing problem.

I’ll land this tomorrow, as-is.

errors: [
{
messageId: 'neverValid',
data: {
reportingValue: 'foo',
attributeName: 'rel',
},
},
{
messageId: 'notPaired',
data: {
reportingValue: 'shortcut',
secondValue: 'foo',
missingValue: 'icon',
},
},
],
},
{
code: '<link rel="shortcut icon"></link>',
output: '<link rel="shortcut icon"></link>',
errors: [
{
messageId: 'spaceDelimited',
data: { attributeName: 'rel' },
},
],
},
{
code: '<link rel="shortcut foo"></link>',
output: '<link rel="shortcut foo"></link>',
errors: [
{
messageId: 'neverValid',
data: {
reportingValue: 'foo',
attributeName: 'rel',
},
},
{
messageId: 'notAlone',
data: {
reportingValue: 'shortcut',
missingValue: 'icon',
},
},
{
messageId: 'spaceDelimited',
data: { attributeName: 'rel' },
},
],
},
{
code: '<a rel="manifest"></a>',
output: '<a rel=""></a>',
Expand Down