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

types(reactivity): disallow using functions with arguments as a computed getter (fix #5692) #5695

Closed
wants to merge 4 commits into from

Conversation

unshame
Copy link

@unshame unshame commented Apr 10, 2022

See #5692

This came up during refactoring: I had a computed that was using a vuex getter.

declare const getItems: () => Item[];

const items = computed(getItems);

Then I refactored getItems to require a parent id.

declare const getItems: (parentId: string) => Item[];

const items = computed(getItems);

And typescript didn't notify me that getItems was used incorrectly.

I ended up banning passing functions into computed directly with no-restricted-syntax eslint rule.

            {
                selector: 'CallExpression[callee.name=computed] > Identifier.arguments',
                message: 'Using computed(callback) is unsafe. Use computed(() => callback()) instead.',
            }

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:

const foo = computed((bar: string) => bar);

@netlify
Copy link

netlify bot commented Apr 10, 2022

Deploy Preview for vuejs-coverage ready!

Name Link
🔨 Latest commit 931f0b8a6b594faae0394e8618b93ddfa1918b25
🔍 Latest deploy log https://app.netlify.com/sites/vuejs-coverage/deploys/62584b55c657bd000801d27a
😎 Deploy Preview https://deploy-preview-5695--vuejs-coverage.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@netlify
Copy link

netlify bot commented Apr 10, 2022

Deploy Preview for vue-sfc-playground ready!

Name Link
🔨 Latest commit 931f0b8a6b594faae0394e8618b93ddfa1918b25
🔍 Latest deploy log https://app.netlify.com/sites/vue-sfc-playground/deploys/62584b5525209500073ea26b
😎 Deploy Preview https://deploy-preview-5695--vue-sfc-playground.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@netlify
Copy link

netlify bot commented Apr 10, 2022

Deploy Preview for vue-next-template-explorer ready!

Name Link
🔨 Latest commit 931f0b8a6b594faae0394e8618b93ddfa1918b25
🔍 Latest deploy log https://app.netlify.com/sites/vue-next-template-explorer/deploys/62584b55461eb2000832aecd
😎 Deploy Preview https://deploy-preview-5695--vue-next-template-explorer.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@yyx990803
Copy link
Member

Unfortunately the relaxed arguments type seems necessary for the type tests to pass.

@unshame
Copy link
Author

unshame commented Apr 13, 2022

I relaxed the type back to what it was but only applied it for computed options. All tests should be passing now.

@@ -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
Copy link
Member

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@unshame
Copy link
Author

unshame commented May 17, 2022

Any chance to get this merged?

export type ComputedSetter<T> = (v: T) => void

export interface WritableComputedOptions<T> {
get: ComputedGetter<T>
set: ComputedSetter<T>
}

export interface WritableComputedOptionsWithVModel<T> {
Copy link
Member

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

Copy link
Author

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
@unshame
Copy link
Author

unshame commented Jul 29, 2022

@yyx990803 any chance to get this merged?

@skirtles-code
Copy link
Contributor

I was wondering whether this PR is still needed since the changes in #9497? That PR introduced the oldValue as an argument, with corresponding changes to the relevant types. The ...args: any[] is now gone.

@unshame unshame closed this Apr 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Rejected
Development

Successfully merging this pull request may close these issues.

None yet

3 participants