-
-
Notifications
You must be signed in to change notification settings - Fork 681
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Closed
Closed
Changes from 1 commit
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 think
a
,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 the
data()
method are sorted.Just clarifying here, we only want to sort the keys of
data()
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
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 following
b
anda
should also be sorted. Because they are inside the Vue component.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 the
vue/sort-keys
rule.I think it would be better to have a separate rule for sorting related to Vue components than
vue/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 an
ignoreGrandchildrenOf
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 think
vue/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 think
runOutsideVue=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 all
props
,computed
props,methods
andwatch
child keys, eg.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.