-
-
Notifications
You must be signed in to change notification settings - Fork 8.4k
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
types(reactivity): disallow using functions with arguments as a computed getter (fix #5692) #5695
Conversation
✅ Deploy Preview for vuejs-coverage ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
✅ Deploy Preview for vue-sfc-playground ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
✅ Deploy Preview for vue-next-template-explorer ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
Unfortunately the relaxed arguments type seems necessary for the type tests to pass. |
I relaxed the type back to what it was but only applied it for computed options. All tests should be passing now. |
packages/reactivity/src/computed.ts
Outdated
@@ -15,14 +15,20 @@ export interface WritableComputedRef<T> extends Ref<T> { | |||
readonly effect: ReactiveEffect<T> | |||
} | |||
|
|||
export type ComputedGetter<T> = (...args: any[]) => T | |||
export type ComputedGetter<T> = () => T | |||
export type ComputedGetterWithVModel<T> = (...args: any[]) => T |
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.
Let's rename this to ComputedGetterWithInstance
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.
Done
656e0f2
to
931f0b8
Compare
Any chance to get this merged? |
packages/reactivity/src/computed.ts
Outdated
export type ComputedSetter<T> = (v: T) => void | ||
|
||
export interface WritableComputedOptions<T> { | ||
get: ComputedGetter<T> | ||
set: ComputedSetter<T> | ||
} | ||
|
||
export interface WritableComputedOptionsWithVModel<T> { |
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 I wasn't more explicit but this needs to be renamed to ...WithInstance
for consistency
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 should be good now.
…k in options API Add special versions of ComputedGetter and WritableComputedOptions that accept view model as argument
Relax type of ComputedGetterWithVModel
931f0b8
to
363d6e9
Compare
@yyx990803 any chance to get this merged? |
I was wondering whether this PR is still needed since the changes in #9497? That PR introduced the |
See #5692
This came up during refactoring: I had a computed that was using a vuex getter.
Then I refactored getItems to require a parent id.
And typescript didn't notify me that getItems was used incorrectly.
I ended up banning passing functions into
computed
directly withno-restricted-syntax
eslint rule.But I don't see why the type for ComputedGetter is declared as it is. Surely there can't be a case where doing the following is valid: