Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork688
Add runOutsideVue option to vue/sort-keys#1866
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
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.
Looks good! Interesting that the option existed before but was unused and undocumented.
I don't think we need to specify in the docs the version in which this "new" option is released, since it wasn't documented before.
tests/lib/rules/sort-keys.js Outdated
return { | ||
c: 3, | ||
a: 1, | ||
b: 2 | ||
} |
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.
Thank you for this PR!
Why is this not reported? I thinka
,b
, andc
are Vue component properties and should be sorted.
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.
Thank you for the review.
I have pushed a change that will ensure objects returned by thedata()
method are sorted.
Just clarifying here, we only want to sort the keys ofdata()
objects and we can leave objects defined withincomputed
,watchers
andmethods
unsorted?
If that is the case then this is all good.
If we want to also sort all new objects defined within a vue instance but not directly attached to the component maybe we should look towards implementing a new option that only sorts keys that are directly attached to the vue instance.
For example
...computed: { // I would like the computed keys to be sorted. a () { // But I do not want to sort new objects defined within my computed. return { b: null, a: null, } },}
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.
Sorry for late reply.
I think this rule should sort the keys of all objects that aren't reserved as Vue component definitions.
Therefore, I think the followingb
anda
should also be sorted. Because they areinside the Vue component.
computed: { a () { return { b: null, a: null, } },}
If you want to sort only the properties of Vue components (e.g. props, data, computed, watch, and methods), I think we can add another rule to exclude it from being checked by thevue/sort-keys
rule.
I think it would be better to have a separate rule for sorting related to Vue components thanvue/sort-keys
rule, likevue/order-in-components
rule.
https://eslint.vuejs.org/rules/order-in-components.html
What do you think?
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.
Hello, sorry for the slow reply, been very busy with work lately.
This rule already accepts anignoreGrandchildrenOf
option that might be applicable here, iirc it only considers grandchildren keys of the same object, not newly created objects. Let me confirm this behaviour when I get a chance and maybe we could look towards making that ignore all grandchildren, not just grandchildren that are directly attached to the grandparent key.
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.
The ignoreGrandchildrenOf option is an option to avoid conflicting with ordering conventions with Vue components.
If we need an option to ignore all grandchildren, I don't think that option should ignore the outside of the Vue component.
I thinkvue/sort-keys
rule should have a role to enforce an order other than the Vue component ordering convention.
I think the order of Vue properties is the ordering convention of Vue components, so I think we need another rule.
I thinkrunOutsideVue=false
is just an option to ignore the outside of Vue. (However, I'm not sure if it's necessary for the user.)
I think it should be another rule if you want to enforce the order of Vue properties.
Where do you want to enforce the order?
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.
@ota-meshi I was hoping to enforce order on allprops
,computed
props,methods
andwatch
child keys, eg.
export default { // These keys would not be enforced by this rule/option combination. name: 'ComponentName', props: { // These keys order would be enforced by this rule/option combination a: { type: String, default: null }, b: { type: String, default: null }, }, computed: { c () { return { // These keys would not be enforced by this rule/option combination. e: null, d: null, } } },}
I agree, I believe a new rule would be more appropriate for this. I have closed this PR for now and if I get a chance in the future I will submit a new PR, thanks.
Ping@lukethompson |
Sorry@FloEdelmann – I have been too busy to pick this one back up. Will close the PR for now and re-open when I get a chance to work on this again 😃 |
Uh oh!
There was an error while loading.Please reload this page.
Address issue raised here#1865
Update
vue/sort-keys
to use existingoptions.runOutsideVue
option, it seems like previously this option did not do anything.If this change gets accepted we should update the docs to state which version this option will be available from 😄