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

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

Merged
merged 2 commits into from
Apr 27, 2022
Merged

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

merged 2 commits into from
Apr 27, 2022

Conversation

MinatoHikari
Copy link
Contributor

resolve #924

@antfu
Copy link
Member

antfu commented Apr 26, 2022

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
Copy link
Contributor Author

MinatoHikari commented Apr 27, 2022

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
Copy link
Contributor

@MinatoHikari

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

The current version (with 8fb7343) 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
Copy link
Contributor Author

MinatoHikari commented Apr 27, 2022

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
Copy link
Contributor

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
Copy link
Contributor Author

MinatoHikari commented Apr 27, 2022

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
Copy link
Contributor Author

So you must use as, it is necessary.

@jacekkarczmarczyk
Copy link
Contributor

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
Copy link
Contributor Author

MinatoHikari commented Apr 27, 2022

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
Copy link
Contributor Author

MinatoHikari commented Apr 27, 2022

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
Copy link
Contributor

jacekkarczmarczyk commented Apr 27, 2022

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

- add another overload for spread readonly array, change their sequence.
- merge MapSources and OldMapSources
@MinatoHikari
Copy link
Contributor Author

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 antfu merged commit 679f5c2 into vuejs:main Apr 27, 2022
@antfu
Copy link
Member

antfu commented Apr 27, 2022

Awesome, thanks!

@a145789
Copy link
Contributor

a145789 commented Apr 28, 2022

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
Copy link
Contributor Author

MinatoHikari commented Apr 28, 2022

这个问题不知道在 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
Copy link
Contributor Author

MinatoHikari commented Apr 28, 2022

这个问题不知道在 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
Copy link
Contributor

a145789 commented Apr 28, 2022

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
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

invalid type inference in watch([ref] as const) since 1.4.10
4 participants