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

useVModels mistakenly marks returned ref as possibly undefined if the property is optional #3546

Closed
7 tasks done
IlyaSemenov opened this issue Nov 13, 2023 · 9 comments
Closed
7 tasks done

Comments

@IlyaSemenov
Copy link

Describe the bug

useVModels mistakenly marks the returned ref object as possibly undefined if the respective property is optional.

This is not correct. The ref is always returned. The ref value is possibly undefined, but not the ref object itself.

Example:

const props = defineProps<{
  foo: string;
  bar?: string;
}>();

const emit = defineEmits<{
  'update:foo': [foo: string];
  'update:bar': [bar: string | undefined];
}>();

const { foo, bar } = useVModels(props, emit);
const bar2 = useVModel(props, 'bar', emit);

foo.value = 'foo'; // OK
bar.value = 'bar'; // 'bar' is possibly 'undefined' <------ Type error (but no runtime error)
bar2.value = 'bar2'; // OK

How to reproduce: in StackBlitz, run npm test.

Reproduction

https://stackblitz.com/edit/nuxt-starter-vj3utk?file=components%2Ftest.vue

System Info

System:
    OS: Linux 5.0 undefined
    CPU: (8) x64 Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz
    Memory: 0 Bytes / 0 Bytes
    Shell: 1.0 - /bin/jsh
  Binaries:
    Node: 18.18.0 - /usr/local/bin/node
    Yarn: 1.22.19 - /usr/local/bin/yarn
    npm: 9.4.2 - /usr/local/bin/npm
    pnpm: 8.9.2 - /usr/local/bin/pnpm
  npmPackages:
    @vueuse/core: ^10.6.1 => 10.6.1

Used Package Manager

npm

Validations

@loongzhu
Copy link
Contributor

Hi! I think this type error has nothing to do with vueuse.
You can use vue's withDefaults to set default values to avoid this type error

const props = withDefaults(defineProps<{
  foo: string
  bar?: string
}>(), {
  bar: undefined,
})

@IlyaSemenov
Copy link
Author

I believe it has to do with vueuse, because it's useVModels coming from vueuse that returns incorrectly typed result, isn't it?

I realise I can work around the problem this way or another; my point is that these workarounds should be included with useVModels typings and not repeated by everyone in user-land code.

@loongzhu
Copy link
Contributor

I found that this error also exists in toRefs.

const { foo, bar } = toRefs(props)

So this is not necessarily caused by useVModels.
What do u think?

@Alfred-Skyblue
Copy link
Member

Alfred-Skyblue commented Nov 15, 2023

When using useVModels, iterate through the object to generate a new object for destructuring. This implies that the property must exist in the object to be iterated, even if it is undefined. Therefore, you should ensure that the property exists in the object, even if it is undefined.

const props: { foo: string; bar: string | undefined } = {
  foo: '',
  bar: undefined,
}

const { foo, bar } = useVModels(props, emit)

foo.value = 'foo' // OK
bar.value = 'bar' // OK

export function useVModels<P extends object, Name extends string, Passive extends boolean>(
props: P,
emit?: (name: Name, ...args: any[]) => void,
options: UseVModelOptions<any, Passive> = {},
): ToRefs<P> {
const ret: any = {}
for (const key in props) {
ret[key] = useVModel(
props,
key,
emit,
options as Parameters<typeof useVModel>[3],
)
}
return ret
}

@IlyaSemenov
Copy link
Author

I found that this error also exists in toRefs.

toRefs is a generic mechanism which is supposed to work with all kind of reactive objects. useVModels on the contrary is a function specifically crafted to work with props and nothing else than props.

So this kind of behaviour is fine and expectable for toRefs but can be safely improved for useVModels.

Therefore, you should ensure that the property exists in the object

Vue already and knowingly does that in defineProps. Props defined with:

const props = defineProps<{
  foo: string;
  bar?: string;
}>();

will always have foo and bar regardless what. Even if you call the component as <my-component /> it will have both props:

[Vue warn]: Missing required prop: "foo"
{
  foo: undefined,
  bar: undefined
}

Since useVModels works with props (and nothing else), I believe it can rely on that behaviour and improve developer experience.

@Alfred-Skyblue
Copy link
Member

We need to clarify that its definition is a TypeScript specification, which means that the bar property may not exist in props. In such a case, for in will not iterate over this property. If it's not iterated, the generated new object will not contain this property. Therefore, we need to explicitly pass undefined to ensure its presence in the object.

Of course, we can also return a Proxy for destructuring, which is sufficient to ensure that the returned value is a Ref<undefined | string>. However, this approach would allow us to add arbitrary values to props, potentially disrupting the structure of props.

@IlyaSemenov
Copy link
Author

Basically, instead of returning ToRefs<P> we might return ToRefs<FixProps<P>> which would fix the original props type to reflect what's inside it (that is, keys always existing even if defined as optional).

Granted, that should be Vue concern in the first place, but if vueuse is a collection of hacks on top of Vue, it might do that on its own. I totally understand if you don't want to do that, though.


The implementation could look like:

type NotPossiblyUndefinedKeys<T> = {
  [K in keyof T]: undefined extends T[K] ? never : K
}[keyof T]

type PossiblyUndefinedKeys<T> = {
  [K in keyof T]: undefined extends T[K] ? K : never
}[keyof T]

type FixProps<T> = {
  [K in NotPossiblyUndefinedKeys<T>]: T[K]
} & {
  [K in PossiblyUndefinedKeys<T>]-?: T[K] | undefined
}

I am not proud of it, it's better to simplify if possible, I just didn't manage to achieve that without splitting keys.

@Alfred-Skyblue
Copy link
Member

This property needs to be iterable to ensure that it can be included in the new object.

@IlyaSemenov
Copy link
Author

This is fixed in upstream: vuejs/core#6421

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

No branches or pull requests

3 participants