The Wayback Machine - https://web.archive.org/web/20201016232320/https://github.com/vuejs/eslint-plugin-vue/pull/997
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

New Rule vue/sort-keys #997

Merged
merged 7 commits into from Feb 16, 2020
Merged

New Rule vue/sort-keys #997

merged 7 commits into from Feb 16, 2020

Conversation

@loren138
Copy link
Contributor

@loren138 loren138 commented Dec 5, 2019

This rule creates a version of sort-keys which is compatible with order-in-components.

This addresses the issue raised in #286 as well fulfilling the request in #601.

@loren138 loren138 force-pushed the loren138:vue-sort-keys branch from 686e59f to ea1a563 Dec 5, 2019
Loren
@loren138 loren138 force-pushed the loren138:vue-sort-keys branch from ea1a563 to 49b4486 Dec 5, 2019
Loren
@@ -2,7 +2,7 @@
/coverage
/node_modules
/tests/fixtures
/tests/integrations/*/node_modules
/tests/integrations/eslint-plugin-import

This comment has been minimized.

@loren138

loren138 Dec 5, 2019
Author Contributor

EsLint 6.7.0 changes eslintignore parsing. The previous one seems to have ignored the whole folder which I'm now explicitly doing. Otherwise, you get errors about eslint-plugin-ignore not being installed when running npm run lint after a clean install.

@ota-meshi
Copy link
Member

@ota-meshi ota-meshi commented Jan 8, 2020

Thank you for this PR.

Could you change to ignore the following Vue component options?

  • props.xxx
    e.g.

    props: {
      foo: {
        type: String,
        default: 'foo'
      }
    }
  • computed.xxx
    e.g.

    computed: {
      foo: {
        get() {},
        set(foo) {}
      }
    }
  • watch.xxx
    e.g.

    watch: {
      foo: {
        handler() {},
        deep: true,
      }
    }
  • directives.xxx
    e.g.

    directives: {
      focus: {
        bind() {},
        inserted() {}
      }
    }
  • components.xxx

  • mixins[*].xxx

  • extends

  • inject.xxx

  • model
    e.g.

    model: {
      prop: 'checked',
      event: 'change'
    }
@loren138
Copy link
Contributor Author

@loren138 loren138 commented Jan 8, 2020

Hi,
I'm not sure I completely understand your request.

My goal was to make sure all the props, computed, data, methods, etc are in alphabetical order while allowing the component order to be set by vue/order-in-components.

That would make this example invalid

props: {
  foo: {
    type: String,
    default: 'foo'
  }
}

it would have to have default before type

props: {
  foo: {
    default: 'foo',
    type: String
  }
}

The main goal, however, was to make this invalid:

props: {
  foo: {
    default: 'foo',
    type: String
  },
  bar: {
    default: 'foo',
    type: String
  },
}

and require it to have bar before foo:

props: {
  bar: {
    default: 'foo',
    type: String
  },
  foo: {
    default: 'foo',
    type: String
  }
}
@ota-meshi
Copy link
Member

@ota-meshi ota-meshi commented Jan 8, 2020

Hi! @loren138 .

It doesn't exist now, but we may add rules to enforce the order of props definition in the future. This already has an issue open #541.

I think we need to ignore the some options in the Vue component so that they don't conflict when adding new rules.

@loren138
Copy link
Contributor Author

@loren138 loren138 commented Jan 8, 2020

In that case, what things would be enforced by this rule? Can you give an example of something that you think would be invalid (while also ignoring all the things you mentioned above)?

@ota-meshi
Copy link
Member

@ota-meshi ota-meshi commented Jan 8, 2020

I think rule #541 will report an error with the following code:

props: {
  foo: {
    default: 'foo',
    type: String // The `type` property should come before the `default` property.
  },
}

The following code is valid:

props: {
  foo: {
    type: String,
    default: 'foo'
  },
}

The following code does not report an error with rule #541.
However, the vue/sort-keys rule reports an error.

props: {
  foo: { },
  bar: { },
}
@loren138
Copy link
Contributor Author

@loren138 loren138 commented Jan 8, 2020

Got it, I'll work on that change.

Loren
@loren138
Copy link
Contributor Author

@loren138 loren138 commented Jan 8, 2020

@ota-meshi I think the latest commit addresses your comments. I made what to ignore configurable in case people don't want to use any of the new rules or want to force alphabetical order until those are ready.

Copy link
Member

@ota-meshi ota-meshi left a comment

Thank you for changing this PR😄

I have some requests.

const reportErrors = (isVue) => {
if (isVue) {
errors = errors.filter((error) => {
let filter = error.hasUpper

This comment has been minimized.

@ota-meshi

ota-meshi Jan 9, 2020
Member

Vue component objects may not be top-level objects. Excluding it with hasUpper can cause false positives.

const obj = {
  foo() {
    Vue.component('my-component', {
      name: 'app',
      data() {}
    })
  }
}

message: "Expected object keys to be in {{natual}}{{insensitive}}{{order}}ending order. '{{thisName}}' should be before '{{prevName}}'.",
parentName: stack.upper && stack.upper.prevName,
grandparentName: stack.upper && stack.upper.upper && stack.upper.upper.prevName,
greatGrandparentName: stack.upper && stack.upper.upper && stack.upper.upper.upper && stack.upper.upper.upper.prevName,

This comment has been minimized.

@ota-meshi

ota-meshi Jan 9, 2020
Member

It may false negatives if the objects are not chained.

export default {
  computed: {
    foo () {
      return {
        b,
        a
      }
    }
  }
}


```json
{
"sort-keys": ["error", "asc", {"caseSensitive": true, "natural": false, "minKeys": 2}]

This comment has been minimized.

@ota-meshi

ota-meshi Jan 9, 2020
Member

Could you added properties be included in the example?

Loren added 2 commits Jan 9, 2020
Loren
Loren
@loren138
Copy link
Contributor Author

@loren138 loren138 commented Jan 9, 2020

@ota-meshi Thanks for catching those issues. They are fixed now. Let me know if you see anymore.

@loren138 loren138 requested a review from ota-meshi Jan 20, 2020
@loren138
Copy link
Contributor Author

@loren138 loren138 commented Jan 20, 2020

@ota-meshi Any further comments on this PR?

@ota-meshi
Copy link
Member

@ota-meshi ota-meshi commented Jan 20, 2020

@loren138 sorry, but I haven't checked it yet.
I'm a little busy now, so please wait.

Copy link
Member

@ota-meshi ota-meshi left a comment

Thank you for the change! I'm sorry to have kept you waiting.

I found a typo. It looks good to me except for this.

docs/rules/sort-keys.md Outdated Show resolved Hide resolved
Co-Authored-By: Yosuke Ota <[email protected]>
@loren138 loren138 requested a review from ota-meshi Jan 28, 2020
Copy link
Member

@ota-meshi ota-meshi left a comment

Thank you!

@ota-meshi ota-meshi merged commit 7608dea into vuejs:master Feb 16, 2020
8 checks passed
8 checks passed
Header rules No header rules processed
Details
Pages changed 105 new files uploaded
Details
Redirect rules No redirect rules processed
Details
Mixed content No mixed content detected
Details
ci/circleci: node-v10 Your tests passed on CircleCI!
Details
ci/circleci: node-v12 Your tests passed on CircleCI!
Details
ci/circleci: node-v8 Your tests passed on CircleCI!
Details
deploy/netlify Deploy preview ready!
Details
@loren138 loren138 mentioned this pull request Feb 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.