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

Add prefer-true-attribute-shorthand rule #1796

Merged
merged 7 commits into from Feb 12, 2022
Merged

Add prefer-true-attribute-shorthand rule #1796

merged 7 commits into from Feb 12, 2022

Conversation

g-plane
Copy link
Contributor

@g-plane g-plane commented Feb 9, 2022

Closes #775
Closes #1781

Copy link
Member

@ota-meshi ota-meshi left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you!

Copy link
Member

@FloEdelmann FloEdelmann left a comment

Choose a reason for hiding this comment

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

Thanks for implementing this! However, I have a few remarks.

In #775 (comment), it was suggested to make it configurable whether to always or never use shorthand props. I think that's a good idea, given some people may prefer to never use shorthand props because of their quirks or just by taste.

The never case could also be auto-fixable, while the suggestion you implemented would be fine for the always case.

lib/rules/prefer-true-attribute-shorthand.js Outdated Show resolved Hide resolved
lib/rules/prefer-true-attribute-shorthand.js Outdated Show resolved Hide resolved
lib/rules/prefer-true-attribute-shorthand.js Outdated Show resolved Hide resolved
docs/rules/prefer-true-attribute-shorthand.md Outdated Show resolved Hide resolved
Copy link
Member

@FloEdelmann FloEdelmann left a comment

Choose a reason for hiding this comment

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

Nice, this looks really good!

Only one problem I just noticed: The never option will also report native HTML shorthand attributes: <input checked> should become <input checked="checked">, not <input :checked="true">. Or is this actually allowed?

If it's allowed, please also add this test case to the "allowed" list to document this behavior.

If it isn't, we need a little more logic: Since one can use native HTML shorthand attributes like hidden also on components (e.g. <MyComponent hidden>), I think we also have to use suggestions instead of an autofix there: One for changing it into a longhand Vue prop, one for changing it into a longhand HTML attribute.

We might be a bit more clever about this and actually do autofix it in one case though: The shorthand attribute is on a known HTML element. Then we know that it should be autofixed to a longhand HTML attribute. But I think this is a nice-to-have improvement; only having suggestions in all cases would be fine for me.

@FloEdelmann
Copy link
Member

@g-plane
Copy link
Contributor Author

g-plane commented Feb 11, 2022

What about skipping native HTML elements, and just check components?

@FloEdelmann
Copy link
Member

That'd be fine for me, but still not catch the <MyComponent hidden> case. hidden might be a prop or an HTML attribute.

@g-plane
Copy link
Contributor Author

g-plane commented Feb 11, 2022

We can turn auto fix into suggestions, as you said.

Copy link
Member

@FloEdelmann FloEdelmann left a comment

Choose a reason for hiding this comment

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

Nice, thanks for the improvements and the test cases! Please run npm run update one more time to update the docs (there is no autofix anymore). But this is not merge-blocking; we can do it afterwards.

@ota-meshi ota-meshi merged commit 1d1be85 into vuejs:master Feb 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants