feat(shared): ensure return types exists #4659
Conversation
| import { getCurrentInstance, provide } from 'vue' | ||
| import { localProvidedStateMap } from './map' | ||
|
|
||
| export type ProvideLocalReturn = void |
There was a problem hiding this comment.
do we gain anything in these cases where the return type is void?
i wonder if this is a waste until we have a non-void return type
There was a problem hiding this comment.
I thought its better for docs, because we have always a consitient return type.
| @@ -1,6 +1,8 @@ | |||
| import { toRefs, toValue } from 'vue' | |||
| import { reactiveComputed } from '../reactiveComputed' | |||
|
|
|||
| export type ReactiveOmitReturn<T extends object, K extends keyof T> = Omit<T, K> | Partial<T> | |||
There was a problem hiding this comment.
how come this one changed? (it used to be Omit<T, K> without the Partial)
There was a problem hiding this comment.
yeah. you are right 😅
There was a problem hiding this comment.
Actually it is Omit or partial.
I have fine tuned the type like this:
type ReactiveOmitOrPartial<T, K extends keyof T | undefined = undefined> =
[K] extends [undefined]
? Partial<T>
: Omit<T, Extract<K, keyof T>>;
type A = ReactiveOmitOrPartial<{a: string}>
type B = ReactiveOmitOrPartial<{a: string, b:number}, 'a'>| @@ -2,14 +2,16 @@ import type { MaybeRefOrGetter, Ref } from 'vue' | |||
| import { customRef, toValue } from 'vue' | |||
| import { tryOnScopeDispose } from '../tryOnScopeDispose' | |||
|
|
|||
| export type RefAutoResetReturn<T = any> = Ref<T> | |||
There was a problem hiding this comment.
i don't think we need a default as we always infer a T earlier
| export type RefAutoResetReturn<T = any> = Ref<T> | |
| export type RefAutoResetReturnType<T> = Ref<T> |
There was a problem hiding this comment.
When a user imports the type I wanted to have a good default behavior.
There was a problem hiding this comment.
they should always be specifying T, though. as it should be inferred from any usages
lets move this to the main thread though as i wrote more in my reply there
|
before i continue reviewing, one change across many of these i would make is around default type params you've changed quite a few places from e.g. |
You are right that we infer this type internally, but if you use the Setting this only simplifies the default case. |
|
do you have an example of someone using the return type like this today? i still wouldn't do it. i think people should be declaring the type they want rather than leaning on |
Maybe this is just a matter of preference, but I think it is good practice to have any as the default for exported types. Lets take an imaginary example for now. export type XyzReturn<T = any> = {
type: Computed<'a' | 'b' | 'c'>
data: Ref<T>
readonly key1: string,
readonly key2: Readonly<ShallowRef<{a: string, b:string}>>
key3: number
}If I have a function or whatever type that uses the // example 1: dont care about data type
function useExp1(): Pick<XyzReturn, 'key1'|'key2'> {}Overall, I think it feels more natural to just write types like |
|
All usages of it are inferred though. Nobody within vueuse would ever specify it manually because it would be inferred By adding a default, you're implying it can sometimes be unknown/not inferred but that isn't true It makes sense where it can't be inferred but that's a rarity in this repo as you pretty much always pass T in. Maybe in fetch or something you don't know up front but it's an edge case and those are the only ones that make sense to have a default Your example isn't something that happens. It would be: function useExp1<T>(input: T): XyzReturn<T>i.e it always comes from an inferred input, making the default useless |
But |
|
It is part of the public API and we infer it publicly, from the input There's no situation where it wouldn't be set unless someone is trying to work around the type system. In those cases they can pass I don't know how better to explain this. It doesn't need a default because we should always have a type, usually an inferred one. |
We cannot always infer the types in userspace. And sometimes the generic types do not even matter. const useMyWebWorker = () => : {whatever: ShallowRef<boolean>} & Pick<UseWebWorkerReturn, 'post' | 'worker'> {
const whatever = shallowRef(false)
const {post, worker} = useWebWorker('/path/to.js')
// ...
return {
whatever,
post,
worker,
}
}I do not even care about the data (generic type) here. Sometimes a user just wants to specify something like interface UseXyzReturn<T = any> {
key1: string
key2: boolean | ()=>boolean
key3: T[]
}
const fn = (a: UseXyzReturn['key1'], b: UseXyzReturn['key2']) => /* */)`fn might not be directly related directly related to the actual data (key3) with the generic type. Overall, I think this is just a personal preference. I am fine with dropping the |
|
in your example, i still don't see a clear example of where the type wouldn't be inferred in all places we consume a declare function useWhatever<T>(input: T): UseWhateverReturn<T>;there's no situation where the return type doesn't have a in places like but thats an edge case. we should have a default when necessary rather than for the sake of it |
Okay, now I see your point. I still kind of disagree 😅. Also not setting defaults might create unnessesary breaking changes if we add a feature that comes with a generic to an existing composable. In this case, we will always break the existing type because we are not setting defaults. |
|
Sorry for the late reply.
I completely agree with this; having a unified definition is really nice :) |
Before submitting the PR, please make sure you do the following
fixes #123).Description
This PR ensures consitent (naming + export) return types for "@vueuse/shared" and "fixes" some minor type issues along the way.
The following composables have still some open questions:
Additional context