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 #1819: Enforce order between script and script setup #1825

Merged
merged 17 commits into from Apr 11, 2022

Conversation

doug-wade
Copy link
Contributor

Fixes #1819 . Adds a new tag type to the component-tags-order, script/setup.

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.

Thank you for this PR!
I wrote comments. I also want @FloEdelmann 's opinion on the option format.

tests/lib/rules/component-tags-order.js Show resolved Hide resolved
docs/rules/component-tags-order.md Outdated Show resolved Hide resolved
lib/rules/component-tags-order.js Outdated Show resolved Hide resolved
@FloEdelmann
Copy link
Member

I also think that existing options should continue to work, i.e. "script" should report both <script> and <script setup>.

I really like the CSS selector format suggestion by @ota-meshi. It's really clear and concise, and allows configuring e.g. <style scoped> above <style>, or any other individual use cases.

The object format is too noisy for me.

@ota-meshi
Copy link
Member

@FloEdelmann Thank you for your opinion.

I really like the CSS selector format suggestion by @ota-meshi. It's really clear and concise, and allows configuring e.g. <style scoped> above <style>, or any other individual use cases.
The object format is too noisy for me.

I agree with you 👍

@doug-wade Can you change this PR so that user can specify a CSS selector as an option?
I think it's a good to add postcss-selector-parser to the dependency and use it to parse the selector.
https://github.com/postcss/postcss-selector-parser

If it's difficult to parse the CSS selector, could you add "script:not([setup])" and "script[setup]" as constant options?
I change the rule to be able to use CSS selectors when I have time.
I've used postcss-selector-parser in another project so I can implement it.

@doug-wade doug-wade requested a review from ota-meshi April 2, 2022 05:55
docs/rules/component-tags-order.md Outdated Show resolved Hide resolved
docs/rules/component-tags-order.md Outdated Show resolved Hide resolved
lib/rules/component-tags-order.js 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.

Looks good to me. Thanks for implementing the feedback!

@ota-meshi can you have another look?

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.

Thank you for changing this PR.
I will follow up about postcss-selector-parser later, so I commented on other things.

lib/rules/component-tags-order.js Outdated Show resolved Hide resolved
docs/rules/component-tags-order.md Outdated Show resolved Hide resolved
tests/lib/rules/component-tags-order.js Outdated Show resolved Hide resolved
tests/lib/rules/component-tags-order.js Outdated Show resolved Hide resolved
tests/lib/rules/component-tags-order.js Outdated Show resolved Hide resolved
tests/lib/rules/component-tags-order.js Outdated Show resolved Hide resolved
doug-wade and others added 7 commits April 10, 2022 15:12
Co-authored-by: Yosuke Ota <otameshiyo23@gmail.com>
Co-authored-by: Yosuke Ota <otameshiyo23@gmail.com>
Co-authored-by: Yosuke Ota <otameshiyo23@gmail.com>
Co-authored-by: Yosuke Ota <otameshiyo23@gmail.com>
Co-authored-by: Yosuke Ota <otameshiyo23@gmail.com>
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.

@ota-meshi ota-meshi merged commit 926064c into vuejs:master Apr 11, 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
Development

Successfully merging this pull request may close these issues.

vue/component-tags-order should enforce order between <script> and <script setup>
3 participants