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.

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

Merged
merged 1 commit into from Sep 21, 2020

Conversation

julienw
Copy link
Contributor

@julienw julienw commented Sep 9, 2020

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?)

@julienw julienw marked this pull request as draft September 9, 2020 17:14
@julienw julienw force-pushed the fix-no-unknown-property branch 2 times, most recently from c70c6b0 to 7791b63 Compare September 9, 2020 17:45
@julienw julienw changed the title [no-unknown-property] Support fixing attributes with any input case Fix no-unknown-property: check attributes with any input case Sep 9, 2020
});
return found ? names[i] : null;
// Let's find a possible attribute match with a case-insensitive search.
const foundName = names.find(
Copy link
Contributor Author

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.

@julienw julienw marked this pull request as ready for review September 9, 2020 17:50
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

LGTM pending question

const foundName = names.find(
(element, index) => element.toLowerCase() === name.toLowerCase()
);
return foundName === undefined ? null : foundName;
Copy link
Member

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?

Copy link
Contributor Author

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" whereas undefined is accidental.

In this context I agree this isn't so useful, where getStandardName is used at only one place. I can change this.

@@ -205,11 +206,11 @@ function getStandardName(name, context) {
}
let i = -1;
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
let i = -1;

this is no longer used

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()
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
(element, index) => element.toLowerCase() === name.toLowerCase()
(element) => element.toLowerCase() === name.toLowerCase()

@julienw
Copy link
Contributor Author

julienw commented Sep 12, 2020

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 <div foo="bar">). Maybe this could be an option though... What do you think?

@ljharb
Copy link
Member

ljharb commented Sep 21, 2020

@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 :-)

Copy link
Member

@ljharb ljharb left a 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.

@@ -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.
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
* @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.

Comment on lines 209 to 212
const foundName = names.find(
(element) => element.toLowerCase() === name.toLowerCase()
);
return foundName; // This can be undefined if the value hasn't been found.
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
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 :-)

@ljharb ljharb merged commit 4da7451 into jsx-eslint:master Sep 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants