-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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.
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
Fix no-unknown-property: check attributes with any input case #2790
Conversation
c70c6b0
to
7791b63
Compare
7791b63
to
93e4585
Compare
lib/rules/no-unknown-property.js
Outdated
}); | ||
return found ? names[i] : null; | ||
// Let's find a possible attribute match with a case-insensitive search. | ||
const foundName = names.find( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked that find
is supported by node 4 before changing the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM pending question
lib/rules/no-unknown-property.js
Outdated
const foundName = names.find( | ||
(element, index) => element.toLowerCase() === name.toLowerCase() | ||
); | ||
return foundName === undefined ? null : foundName; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any reason not to just return foundName
here, and have the function return String | undefined
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was 2 reasons:
- initially I wanted to keep it working just like before, without changing too much code -- I ended up changing more code than I wanted :)
- we have this habit in our project that we use
null
as an explicite "no value" whereasundefined
is accidental.
In this context I agree this isn't so useful, where getStandardName
is used at only one place. I can change this.
lib/rules/no-unknown-property.js
Outdated
@@ -205,11 +206,11 @@ function getStandardName(name, context) { | |||
} | |||
let i = -1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let i = -1; |
this is no longer used
lib/rules/no-unknown-property.js
Outdated
return found ? names[i] : null; | ||
// Let's find a possible attribute match with a case-insensitive search. | ||
const foundName = names.find( | ||
(element, index) => element.toLowerCase() === name.toLowerCase() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(element, index) => element.toLowerCase() === name.toLowerCase() | |
(element) => element.toLowerCase() === name.toLowerCase() |
93e4585
to
f7a072a
Compare
Thanks for the review @ljharb, I think I fixed all your comments! One question: I think this property should also check completely unknown attributes on the html tags (eg |
@julienw yes, ideally it should, but we'd need to ensure we can use the same mappings React itself uses, and i don't think they're exposed anywhere. I think we can do that in another PR :-) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I'll clean up these tweaks and land.
lib/rules/no-unknown-property.js
Outdated
@@ -194,7 +195,7 @@ function tagNameHasDot(node) { | |||
* Get the standard name of the attribute. | |||
* @param {String} name - Name of the attribute. | |||
* @param {String} context - eslint context | |||
* @returns {String} The standard name of the attribute. | |||
* @returns {String | undefined} The standard name of the attribute, or null if no standard name was found. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* @returns {String | undefined} The standard name of the attribute, or null if no standard name was found. | |
* @returns {String | undefined} The standard name of the attribute, or undefined if no standard name was found. |
lib/rules/no-unknown-property.js
Outdated
const foundName = names.find( | ||
(element) => element.toLowerCase() === name.toLowerCase() | ||
); | ||
return foundName; // This can be undefined if the value hasn't been found. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const foundName = names.find( | |
(element) => element.toLowerCase() === name.toLowerCase() | |
); | |
return foundName; // This can be undefined if the value hasn't been found. | |
return names.find( | |
(element) => element.toLowerCase() === name.toLowerCase() | |
); |
the comment isn't really helpful since that's how .find
already works :-)
f7a072a
to
4da7451
Compare
I think this is ready now. I checked my project with this rule and it didn't break anything :-)
I wasn't sure if this rule should also check completely unknown attributes. My view is that it should, but I didn't do it in this patch.
With this patch the rule will probably find new errors in existing code, so it should probably be versioned in a new minor or major version.
Tell me what you think!
(pretest fails but this seems to happen on master too?)