- Sponsor
-
Notifications
You must be signed in to change notification settings - Fork 681
Add composition api's computed function support to vue/no-side-effects-in-computed-properties close #1393 #1407
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
Add composition api's computed function support to vue/no-side-effects-in-computed-properties close #1393 #1407
Conversation
…s-in-computed-properties close vuejs#1393
} | ||
|
||
const def = variable.defs[0] | ||
if ( |
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 think it's better to report only the variables defined in the setup()
function scope. There can be many false negatives, but I think there are many false positives in the current process.
e.g.
const myUtils = { ... } // My utilities
export default {
setup(props) {
const array = reactive([])
const foo = computed(() => myUtils.reverse(array)) // OK Build an array in reverse order.
const bar = computed(() => array.reverse()) // NG
const baz = computed(() => myUtils.reverse(props.array)) // OK Build an array in reverse order.
const qux = computed(() => props.array.reverse()) // NG
}
}
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 think the problem only lies with the CallExpressions.
I changed the code to only report variables which are declared inside setup functions.
But maybe it is possible to report mutations that are not by CallExpressions even if it is declared outside.
const myUtils = {}
myUtils.reverse() // this is ok, since it is unknown whether it is destructive
myUtils = null // this is a mutation
myUtils.count++ // this is a mutation
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 main purpose of this rule is to prevent a reactive loop due to mutation. Therefore, I don't think variables that are unlikely to be reactive should be reported.
I think it is possible to add options to check for other mutations.
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 see. Thank you for answering!
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 reviewing! I fixed it.
} | ||
|
||
const def = variable.defs[0] | ||
if ( |
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 think the problem only lies with the CallExpressions.
I changed the code to only report variables which are declared inside setup functions.
But maybe it is possible to report mutations that are not by CallExpressions even if it is declared outside.
const myUtils = {}
myUtils.reverse() // this is ok, since it is unknown whether it is destructive
myUtils = null // this is a mutation
myUtils.count++ // this is a mutation
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. Thank you for changing. LGTM!
This PR adds support for composition api's computed function to
vue/no-side-effects-in-computed-properties
.close #1393
With computed properties, this rule only warned about mutating
this
.But since computed functions does not use
this
, I made it warn about mutating any variables which are not declared inside the computed function.For example
Is this approach ok? Any other one is better?
If it is ok, should I change the behavior with computed properties to this one?