Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Add block-attributes-order rule #1973

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

Closed
Kingwl wants to merge 10 commits into vuejs:master from Kingwl:feat/block-attributes-orders

Conversation

Copy link
Member

@Kingwl Kingwl commented Sep 16, 2022
edited
Loading

Fixes #1970

paescuj and Cherepanov-Pavel reacted with heart emoji
@Kingwl Kingwl force-pushed the feat/block-attributes-orders branch from 7c9babb to 31666df Compare September 16, 2022 07:06
@Kingwl Kingwl marked this pull request as ready for review September 16, 2022 07:22
Copy link
Contributor

mesqueeb commented Sep 17, 2022
edited
Loading

@Kingwl thanks for the PR!

is the default order going to be

<script setup lang="ts"></script>

OR

<script lang="ts" setup></script>

it's hard to tell from the PR markdown file alone.

PS: I think the former is better for consistency with official Vue documentation

Copy link
Member Author

Kingwl commented Sep 19, 2022
edited
Loading

Added the default orders into markdown.

AFAIK, there's no official Vue recommended block attributes order.

@sodatea Any suggestions?

Copy link
Contributor

@Kingwl https://vuejs.org/guide/typescript/composition-api.html

Official Vue docs use <script setup lang="ts">

So I think having the same default as that seems best : )

Copy link
Member Author

Kingwl commented Sep 19, 2022
edited
Loading

Done.

And make style block has xxx yyy lang src order too (for the consistency).


This rule aims to enforce ordering of block attributes.

### The default order
Copy link
Member

@ota-meshi ota-meshi Sep 20, 2022

Choose a reason for hiding this comment

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

I think we need to discuss option formats.

If we want to be more granular and configurable by the user, I suggest using an array of objects with elements and ordering, like this:

{'vue/block-attributes-order': ['error',
 [
 { element: 'script', order: ['setup, 'lang', 'src'] },
 { element: 'template', order: ['functional', 'lang', 'src'] },
 { element: 'style', order: ['scoped', 'module', 'lang', 'src'] },
 { element: 'i18n', order: ['global', 'locale', 'lang', 'src'] },
 ]
]}

You can use CSS selectors for the element property. This is similar to the vue/component-tags-order option.
The order option specifies the order of the attributes. It can use attribute names, regular expressions and their arrays, and negation. This is similar to the svelte/sort-attributes option.

If you want lang and src to always be at the back:

{'vue/block-attributes-order': ['error',
 [
 { element: '*', order: [['/.*/', '!/^(?:src|lang)$/'], 'lang', 'src'] }
 ]
]}

Patterns can be used to correct some order even for unknown blocks and unknown attributes.

What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated as comments.

@ota-meshi ota-meshi marked this pull request as draft October 4, 2022 04:32
@Kingwl Kingwl marked this pull request as ready for review October 14, 2022 10:02

This comment was marked as off-topic.

Comment on lines +59 to +67
{
element: 'style',
order: [
WELL_KNOWN_STYLE_ATTRS.module,
WELL_KNOWN_STYLE_ATTRS.scoped,
WELL_KNOWN_STYLE_ATTRS.lang,
WELL_KNOWN_STYLE_ATTRS.src
]
},
Copy link
Member

@FloEdelmann FloEdelmann Oct 19, 2022
edited
Loading

Choose a reason for hiding this comment

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

I think you can inline the constants since they are only used here. I.e.:

 {
 element: 'style',
 order: ['module', 'scoped', 'lang', 'src']
 },

sidebarDepth: 0
title: vue/block-attributes-order
description: enforce order of block attributes
since: v4.3.0
Copy link
Member

@FloEdelmann FloEdelmann Oct 19, 2022

Choose a reason for hiding this comment

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

This rule is not yet released. Please remove this line and re-run npm run update.

Comment on lines +191 to +199
const matchedOption = normalizedOrderOptions.find((option) =>
matchOrderOption(option, element)
)
if (matchedOption) {
return matchedOption
}
return normalizedDefaultOptions.find((option) =>
matchOrderOption(option, element)
)
Copy link
Member

@FloEdelmann FloEdelmann Oct 19, 2022

Choose a reason for hiding this comment

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

I don't understand why there is an if and the same function call again otherwise. Could it not be simplified to this?

Suggested change
const matchedOption = normalizedOrderOptions.find((option) =>
matchOrderOption(option, element)
)
if (matchedOption) {
return matchedOption
}
return normalizedDefaultOptions.find((option) =>
matchOrderOption(option, element)
)
return normalizedDefaultOptions.find((option) =>
matchOrderOption(option, element)
)

Comment on lines +88 to +99
{
filename: 'test-attributes-options-with-unknown-attribute.vue',
code: '<template functional foo lang="pug" src="./template.html"></template>',
options: [
[
{
element: 'template',
order: ['functional', 'foo', 'lang', 'src']
}
]
]
},
Copy link
Member

@FloEdelmann FloEdelmann Oct 19, 2022
edited
Loading

Choose a reason for hiding this comment

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

The test file name doesn't match the code/options here.

},
{
filename: 'test-attributes-options-not-specify-some-attribute-2.vue',
code: '<style module scoped src="./style.css" lang="less"></style>',
Copy link
Member

@FloEdelmann FloEdelmann Oct 19, 2022
edited
Loading

Choose a reason for hiding this comment

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

I think that unknown attributes should only be allowed at the end. This would simplify the rule logic and make some test cases obsolete.

Comment on lines +218 to +221
{
type: 'VAttribute',
message: 'Attribute "lang" should go before "src".'
},
Copy link
Member

@FloEdelmann FloEdelmann Oct 19, 2022

Choose a reason for hiding this comment

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

Please add line and column of the reported error to all the expected errors in this file. However, you can also remove the type: 'VAttribute' everywhere.

Comment on lines +91 to +93
function ordersToRegexOrders(item) {
return Array.isArray(item) ? item.map((x) => new RegExp(x)) : new RegExp(item)
}
Copy link
Member

@ota-meshi ota-meshi Oct 26, 2022

Choose a reason for hiding this comment

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

Could you change it to accept more complex options by doing the following?

Suggested change
function ordersToRegexOrders(item) {
return Array.isArray(item) ? item.map((x) => new RegExp(x)) : new RegExp(item)
}
const { toRegExp } = require('../utils/regexp')
/**
* @param {string | string[]} item
* @return {(str: string) => boolean}
*/
function ordersToMatcher(item) {
return Array.isArray(item) ? compileMatcher(item) : compileMatcher([item])
}
/**
* Compile matcher
* @param {string[]} pattern
* @return {(str: string) => boolean}
*/
function compileMatcher(pattern) {
const rules = []
for (const p of pattern) {
let negative, patternStr
if (p.startsWith("!")) {
// If there is `!` at the beginning, it will be parsed with a negative pattern.
negative = true
patternStr = p.substring(1)
} else {
negative = false
patternStr = p
}
const regex = toRegExp(patternStr)
rules.push({ negative, match: (str) => regex.test(str) })
}
return (str) => {
// If the first rule is a negative pattern, they are considered to match if they do not match that pattern.
let result = Boolean(rules[0]?.negative)
for (const { negative, match } of rules) {
if (result === !negative) {
// Even if it matches, the result does not change, so skip it.
continue
}
if (match(str)) {
result = !negative
}
}
return result
}
}

This change allows users to use pattern formats such as:

  • "foo" ... Matches only the foo attribute name.
  • "/foo/" ... Matches attribute names that match the /foo/ regex. That is, it matches the attribute name including foo.
  • "!foo" ... Exclude foo attribute from the matched attribute names. When used first in the array or alone, matches other than the foo attribute name.
  • "!/foo/" ... Excludes attributes that match the /foo/ regex from the matched attribute names. When used first in the array or alone, matches an attribute name that does not match the /foo/ regex.
  • ["foo", "/^bar/u"] ... Matches the foo attribute or the attribute name that matches the /^bar/u regex.
  • ["/^foo/u", "!foo-bar", "/^baz/u"] ... Matches an attribute name that matches /^foo/u and other than foo-bar, or an attribute name that matches /^baz/u.

I think the rule of other plugin will also be helpful.
https://ota-meshi.github.io/eslint-plugin-svelte/rules/sort-attributes/

type: 'string'
},
order: {
type: 'array',
Copy link
Member

@ota-meshi ota-meshi Oct 26, 2022

Choose a reason for hiding this comment

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

Could you please define the schema so that at least one item is set?

{
type: 'array',
items: {
type: 'object',
Copy link
Member

@ota-meshi ota-meshi Oct 26, 2022

Choose a reason for hiding this comment

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

Could you define the schema so that the two properties are required?

Comment on lines +79 to +81
WELL_KNOWN_TEMPLATE_ATTRS.functional,
WELL_KNOWN_TEMPLATE_ATTRS.lang,
WELL_KNOWN_TEMPLATE_ATTRS.src
Copy link
Member

@ota-meshi ota-meshi Oct 26, 2022

Choose a reason for hiding this comment

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

After changing it to accept complex ordering, could you change it so that lang and src are always ordered last?

e.g.

Suggested change
WELL_KNOWN_TEMPLATE_ATTRS.functional,
WELL_KNOWN_TEMPLATE_ATTRS.lang,
WELL_KNOWN_TEMPLATE_ATTRS.src
"functional",
["!lang", "!src"],
"lang",
"src"

Change other elements in the same way.

WELL_KNOWN_TEMPLATE_ATTRS.src
]
}
])
Copy link
Member

@ota-meshi ota-meshi Oct 26, 2022

Choose a reason for hiding this comment

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

Could you add the following item to force the order of the custom elements as well?

 {
 element: '*',
 order: [
 ["!lang", "!src"],
 "lang",
 "src"
 ]
 },

@ota-meshi ota-meshi marked this pull request as draft January 12, 2023 02:23
Copy link
Contributor

@Kingwl friendly bump : )

Copy link

paescuj commented Jul 1, 2023

Hey there, would be great to have this rule 👍 Happy to help! I can also come up with a new pull request if that is preferred.

Copy link
Member

From our side, a new PR replacing this one would be fine.

xiaoxiangmoe reacted with laugh emoji

Copy link
Member

I'll close this for now as it seems abandoned and there are already some merge conflicts. Feel free to open another PR 🙂

@paescuj this is your chance! 😉

xiaoxiangmoe reacted with thumbs up emoji

Copy link
Contributor

@FloEdelmann I was looking for this rule. Has it been implemented since?

Cherepanov-Pavel reacted with thumbs up emoji

Copy link
Member

No

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FloEdelmann FloEdelmann FloEdelmann requested changes

@ota-meshi ota-meshi ota-meshi requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

tag-attr-order <script lang="ts" setup> vs <script setup lang="ts">

AltStyle によって変換されたページ (->オリジナル) /