vuejs / composition-api

Composition API plugin for Vue 2
https://composition-api.vuejs.org/
MIT License
4.19k stars 343 forks source link

fix(type): align watch types with vue3 #927

Closed MinatoHikari closed 2 years ago

MinatoHikari commented 2 years ago

resolve https://github.com/vuejs/composition-api/issues/924

antfu commented 2 years ago

It does not seem to be a complete fix, ideally the as const shouldn't be required for known array

https://github.com/vuejs/core/blob/d2d27b2e6666d9f4325b8497134bb36267769408/test-dts/watch.test-d.ts#L13-L16

MinatoHikari commented 2 years ago

It does not seem to be a complete fix, ideally the as const shouldn't be required for known array

https://github.com/vuejs/core/blob/d2d27b2e6666d9f4325b8497134bb36267769408/test-dts/watch.test-d.ts#L13-L16

Sorry but I can't get your point? The usage of as const here is making the array be treated as a tuple like object by typescript, then ts can exactly get the correct type of each member.

Without as const we can only get a Union Types like sting | numer

you can try

// const array
const arr = [source, source2, source3] as const
watch(arr, (values, oldValues) => {
  expectType<string>(values[0])
  expectType<string>(values[1])
  expectType<number>(values[2])
  expectType<string>(oldValues[0])
  expectType<string>(oldValues[1])
  expectType<number>(oldValues[2])
})

without as const we will get type error: Argument of type 'string | number' is not assignable to parameter of type 'string'.

jacekkarczmarczyk commented 2 years ago

@MinatoHikari

Without as const we can only get a Union Types like sting | numer allows you to remove as const

The current version (with https://github.com/vuejs/composition-api/commit/8fb7343dddeb63a4401067a65979b0f7de9f7e07) allows you to do

watch([ref1, ref2], ([r1, r2]) => ...)

and infers the type of r1 and r2 correctly, but that change broke the behaviour of

watch([ref1, ref2] as const, ([r1, r2]) => ...)
MinatoHikari commented 2 years ago

I get it. If we don't add as const, the array is treated as Array, the number of watch source can be dynamicly changed, so string | number is right, since you can never know the type of next new menber.

Now I find another problem in vue3

const testRef = ref(4);
const testRef2 = ref(4);
const testRef3 = ref(4);
const arr1 = [testRef2, testRef3];
watch(arr1, ([foo, bar, ...rest]) => {
            console.log(foo);
            console.log(rest);
        });

const addMember = () => {
            arr1.push(testRef);
        };

const newMemberChange = () => {
            testRef.value++;
        };

const originMemberChange = () => {
            testRef2.value++;
        };

return { addMember, newMemberChange, oranginMemberChange }

Run addMember, then run newMemberChange, watch callback won't be called. But if we run addMember->newMemberChange->oranginMemberChange->newMemberChange. Finally watch callback will be called, and after the first time oranginMemberChange being called, whatever times we call addMember, watch with dynamic watchsources normally works.

jacekkarczmarczyk commented 2 years ago

If we don't add as const, the array is treated as Array, the number of watch source can be dynamicly changed, so string | number is right, since you can never know the type of next new menber.

You're talking about different example that in the issue. Your example is

const arr = [whatever];
watch(arr, ([...]) => {

and the example in the issue is

watch([whatever], ([...]) => {
MinatoHikari commented 2 years ago

You're talking about different example that in the issue. Your example is

const arr = [whatever];
watch(arr, ([...]) => {

and the example in the issue is

watch([whatever], ([...]) => {

It's a type problem, even if you const an array in this way, yes you can't change the number of it's member, but it is still be treated as Array by typescript defaultly , not a tuple nor a Readonly<[...]>.

Only when you giving a tunple type to array, it becomes to a tunple, in other case typescript don't know it is a tuple.

An array which will never be changed, but it's type is still Array, not a tuple

MinatoHikari commented 2 years ago

So you must use as, it is necessary.

jacekkarczmarczyk commented 2 years ago

So you're saying that it's not possible that the following code works?

watch([ref(0), ref('')], ([a, b]) => {
  a.toFixed(3);
  b.length;
});

?

If it's not possible then why it's working now?

image

Otherwise I'm not sure what your point is

MinatoHikari commented 2 years ago

So you're saying that it's not possible that the following code works?

watch([ref(0), ref('')], ([a, b]) => {
  a.toFixed(3);
  b.length;
});

?

If it's not possible then why it's working now?

image

Otherwise I'm not sure what your point is

I'm saying type of watch function in current version is wrong. It violates the typescript's default behavior. If your input is [ref(1),ref(2)], Ref<number | string>[] is the correct output type (Array<Ref<string | number>>), not [Ref<sting>,Ref<number>] (tunple type)

MinatoHikari commented 2 years ago

Typescript's type inference never give us a tunple type unless we give an array tuple type at beginning. If you want to use a tunple type, you must explicitly declare it. I think the type of watch in current version break the original intention of typescript, the forced, unexplicitly type transition is counterintuitive.

That's my opinion. Sorry for poor english.

Of course, if typescript can make come changes on the inference of tuple in future, that is the best result.

jacekkarczmarczyk commented 2 years ago

I see your point although I don't agree with it, but the main thing is the you titled your PR "align watch types with vue3" - which suggests that watch types should be aligned with vue3 and vue3 allows both watch([ref1, ref], ...) and watch([ref1, ref2] as const, ...) (in both cases the type of the callback parameters are [number, string] and not (number|string)[]). Currently with this PR the behaviour of vue2 and vue3 are different.

So in my opinion if you think that vue3 is wrong you can open an issue and PR there, but for now making vue2 and vue3 working the same way is more important

MinatoHikari commented 2 years ago

That's right, add another overload for watch, add more test cases ,do some change to make it works for all test cases. Hope Typescript can finally export a generic Tunple type, then so many people won't be fucked by type gymnastics.

antfu commented 2 years ago

Awesome, thanks!

a145789 commented 2 years ago

I get it. If we don't add as const, the array is treated as Array, the number of watch source can be dynamicly changed, so string | number is right, since you can never know the type of next new menber.

Now I find another problem in vue3

const testRef = ref(4);
const testRef2 = ref(4);
const testRef3 = ref(4);
const arr1 = [testRef2, testRef3];
watch(arr1, ([foo, bar, ...rest]) => {
            console.log(foo);
            console.log(rest);
        });

const addMember = () => {
            arr1.push(testRef);
        };

const newMemberChange = () => {
            testRef.value++;
        };

const originMemberChange = () => {
            testRef2.value++;
        };

return { addMember, newMemberChange, oranginMemberChange }

Run addMember, then run newMemberChange, watch callback won't be called. But if we run addMember->newMemberChange->oranginMemberChange->newMemberChange. Finally watch callback will be called, and after the first time oranginMemberChange being called, whatever times we call addMember, watch with dynamic watchsources normally works.

这个问题不知道在 vue3 core 仓库有相关 issues 么?我不知道我有没有理解问题,我觉得这个应该是 watch 监听源是 Array 还是 tuple 的问题,如果已经确定 watch 监听源 是一个 tuple 类型,那原先的写法是对的

type A = [string,number]

const a: A = ['1',1] as const // 我们肯定不会这么写

但我不确信 watch 监听源应该是一个不确定的 Array? 这个官方文档好想也没有具体说明。

MinatoHikari commented 2 years ago

这个问题不知道在 vue3 core 仓库有相关 issues 么?我不知道我有没有理解问题,我觉得这个应该是 watch 监听源是 Array 还是 tuple 的问题,如果已经确定 watch 监听源 是一个 tuple 类型,那原先的写法是对的

type A = [string,number]

const a: A = ['1',1] as const // 我们肯定不会这么写

但我不确信 watch 监听源应该是一个不确定的 Array? 这个官方文档好想也没有具体说明。

ts类型不会影响代码逻辑, 到了js都是array 此处的例子不太恰当,我整了一个控制变量更精确的

        const source2 = ref(2);
        const source3 = ref(1);
        const array = [source2, source3];

        watch(
            array,
            ([r2, r3, ...rest]) => {
                console.log(rest);
            },
            {
                onTrack(r) {
                    console.log('onTrack', r);
                },
            },
        );

        const pushV = () => {
            array.push(ref(0));
        };

        const changeNew = () => {
            array[array.length-1].value++
        };

        const changeOld = () => {
            array[array.length-2].value++;
        };

想了想产生这个现象的原因是传给watch的deps是在依赖项触发onTrigger的时候重新收集,而对此处的watchsource数组操作并不会触发依赖的重新收集,如果要在数组操作时立即更新依赖,可能需要在外面加一层proxy,来重写数组操作,添加更新依赖的代码

MinatoHikari commented 2 years ago

这个问题不知道在 vue3 core 仓库有相关 issues 么?我不知道我有没有理解问题,我觉得这个应该是 watch 监听源是 Array 还是 tuple 的问题,如果已经确定 watch 监听源 是一个 tuple 类型,那原先的写法是对的

type A = [string,number]

const a: A = ['1',1] as const // 我们肯定不会这么写

但我不确信 watch 监听源应该是一个不确定的 Array? 这个官方文档好想也没有具体说明。

ts类型不会影响代码逻辑, 到了js都是array 此处的例子不太恰当,我整了一个控制变量更精确的

         const source2 = ref(2);
         const source3 = ref(1);
         const array = [source2, source3];

         watch(
             array,
             ([r2, r3, ...rest]) => {
                 console.log(rest);
             },
             {
                 onTrack(r) {
                     console.log('onTrack', r);
                 },
             },
         );

         const pushV = () => {
             array.push(ref(0));
         };

         const changeNew = () => {
             array[array.length-1].value++
         };

         const changeOld = () => {
             array[array.length-2].value++;
         };

想了想产生这个现象的原因是传给watch的deps是在依赖项触发onTrigger的时候重新收集,而对此处的watchsource数组操作并不会触发依赖的重新收集,如果要在数组操作时立即更新依赖,可能需要在传进watch的数组外面加一层proxy,来重写数组操作,添加更新依赖的代码

a145789 commented 2 years ago

ts类型不会影响代码逻辑, 到了js都是array 此处的例子不太恰当,我整了一个控制变量更精确的

我懂这个意思,但这个和我说的不冲突,比如

Ts

function fn(p: number){}
fn(1) // work
fn('2') // err

Js

import fn form '';
fn(1) // work
fn('2') // work

ts 会限制参数类型,但在 js 世界传其他类型也可以,此处希望为一个 number 类型,在 js 就是一种行为约定了。我想的是 vuewatch 监听源是不是这样约定限制,即使是在 js 世界,可以传任何参数,但这里期待的参数是一个不可变更的确定数组,超出这个期待的参数可能就不属于 vue 的功能范畴了。

这上面是我个人想法,但我觉得这样可能比较合理一点。

MinatoHikari commented 2 years ago

按照现在的类型推断是无法推断出上面代码的 ...rest 类型的,如果加了 ...rest 其余参数的类型也都会出问题。 要做这个推导应该还需要引入另一个泛型参数。 如果按照现在的类型推导和官网的用例来看,似乎的确默认传进去的数组是不会改变长度的。 但是并不能排除一开始没想过这个数组长度还会变化的情况,这也很正常。