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 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
lukethompson wants to merge 3 commits into vuejs:master from lukethompson:sort-keys-run-outside-vue
Closed

Add runOutsideVue option to vue/sort-keys #1866

lukethompson wants to merge 3 commits into vuejs:master from lukethompson:sort-keys-run-outside-vue

Conversation

Copy link

@lukethompson lukethompson commented Apr 23, 2022
edited
Loading

Address issue raised here #1865

Update vue/sort-keys to use existing options.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 😄

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! 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.

Comment on lines 523 to 527
return {
c: 3,
a: 1,
b: 2
}
Copy link
Member

@ota-meshi ota-meshi May 2, 2022

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, and c are Vue component properties and should be sorted.

Copy link
Author

@lukethompson lukethompson May 2, 2022

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 within computed, watchers and methods 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,
 }
 },
}

Copy link
Member

@ota-meshi ota-meshi May 11, 2022

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 and a should also be sorted. Because they are inside 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 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, like vue/order-in-components rule.
https://eslint.vuejs.org/rules/order-in-components.html
What do you think?

Copy link
Author

@lukethompson lukethompson May 21, 2022

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.

Copy link
Member

@ota-meshi ota-meshi May 28, 2022

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?

Copy link
Author

@lukethompson lukethompson Nov 27, 2022

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 and watch 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.

@FloEdelmann FloEdelmann linked an issue May 7, 2022 that may be closed by this pull request
@ota-meshi ota-meshi marked this pull request as draft August 23, 2022 11:05
Copy link
Member

Copy link
Author

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 😃

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

@FloEdelmann FloEdelmann FloEdelmann approved these changes

@ota-meshi ota-meshi Awaiting requested review from ota-meshi

+1 more reviewer

@Mighty-Tulip Mighty-Tulip Mighty-Tulip approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

vue/sort-keys runOutsideVue option

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