-
-
Notifications
You must be signed in to change notification settings - Fork 805
feat(analyze/js/vue): add noVueArrowFuncInWatch
#8602
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
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 361fe64 The changes in this PR will be included in the next version bump. This PR includes changesets to release 13 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
a8e3c24 to
67c592b
Compare
ematipico
left a comment
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.
This rule would benefit from more tests
| /// } | ||
| /// </script> | ||
| /// ``` | ||
| /// |
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.
It would be nice to add a link to the official docs
| impl Rule for NoVueArrowFuncInWatch { | ||
| type Query = Semantic<AnyPotentialVueComponent>; | ||
| type State = JsArrowFunctionExpression; | ||
| type Signals = Box<[Self::State]>; |
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.
I don't think we need a box here. A Vec should be enough
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.
Every other rule like this uses a Box though
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.
True that, but I'm not sure we ever proved that it's faster. It's fine, we can keep it.
However , why do we need multiple signals? Why isn't Option<State> enough for this rule?
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.
It's how our Vue component querying works currently. We query for something that looks like a component, and then we can extract things from it. This makes it so we don't flag things that aren't in vue components.
It would be better if we could provide better queries though.
| handler: (val, oldVal) => { | ||
| console.log('new: %s, old: %s', val, oldVal) | ||
| } |
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.
Add a test with arrow functions without body e.g.
{
foo: () => ()
}| /* should generate diagnostics */ | ||
| export default { | ||
| watch: { | ||
| foo: (val, oldVal) => { |
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.
Can we add more cases?
- async token
- TypeScript types
- generator token
67c592b to
361fe64
Compare
391e88a to
9a8c98d
Compare

Summary
This adds
noVueArrowFuncInWatch, a port of https://eslint.vuejs.org/rules/no-arrow-functions-in-watch.htmlOnly the code fix was generated by AI.
Test Plan
snapshots, also checked the test cases for the source rule
Docs