-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
feat(useDateFormat): add generic to return type #4719
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
base: main
Are you sure you want to change the base?
Conversation
Can you explain a bit more about when generics are needed? Thanks. |
@ilyaliao Hi, I added generic for better compatibility with other libraries. In particular, I work with primevue and I take some components (for example, datepicker) as the first argument (modelValue) Date and not string, but format as the second argument, which actually equates Date to string. I understand that this sounds like a primevue problem, but I'm sure that primevue is not the only library that works on a similar principle, so it's easier and more beautiful to add generic to vueuse.& |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
meant to request changes, not approve 🫠
Co-authored-by: Robin <robin.kehl@singular-it.de>
i'd like to see an example of the usage that lead to this e.g. you say you bump into it in primevue. can you show us?
|
@43081j Please look at this primevue file. on 519 line you can see that modelValue has type
|
the type error is right, though.
you would need to turn it back into a |
Yes, i undertstand this but i think a code like this |
it would be an error, though: localDate.value; // Date | Date[]
useDateFormat(new Date()); // string
localDate.value = useDateFormat(new Date()); // type error, we're trying to assign a string to a Date your change here is to define the return type of when you write |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there is still a use case for this generic. But the return should extend a string.
Example:
type Month = "jan" | "feb" ...
const month = useDateFormat<Month>(useNow(), 'MMM')
@@ -119,6 +119,6 @@ export type UseDateFormatReturn = ComputedRef<string> | |||
* @param formatStr - The combination of tokens to format the date | |||
* @param options - UseDateFormatOptions | |||
*/ | |||
export function useDateFormat(date: MaybeRefOrGetter<DateLike>, formatStr: MaybeRefOrGetter<string> = 'HH:mm:ss', options: UseDateFormatOptions = {}): UseDateFormatReturn { | |||
export function useDateFormat<T = string>(date: MaybeRefOrGetter<DateLike>, formatStr: MaybeRefOrGetter<string> = 'HH:mm:ss', options: UseDateFormatOptions = {}): UseDateFormatReturn<T> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
export function useDateFormat<T = string>(date: MaybeRefOrGetter<DateLike>, formatStr: MaybeRefOrGetter<string> = 'HH:mm:ss', options: UseDateFormatOptions = {}): UseDateFormatReturn<T> { | |
export function useDateFormat<T extends string = string>(date: MaybeRefOrGetter<DateLike>, formatStr: MaybeRefOrGetter<string> = 'HH:mm:ss', options: UseDateFormatOptions = {}): UseDateFormatReturn<T> { |
@@ -109,7 +109,7 @@ export function normalizeDate(date: DateLike) { | |||
return new Date(date) | |||
} | |||
|
|||
export type UseDateFormatReturn = ComputedRef<string> | |||
export type UseDateFormatReturn<T = string> = ComputedRef<T> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
export type UseDateFormatReturn<T = string> = ComputedRef<T> | |
export type UseDateFormatReturn<T extends string = string> = ComputedRef<T> |
I don't think that's what the OP was trying to do though 😬 so it's likely this can be closed once there's understanding why it doesn't make sense to do the change |
Yes, but with |
i dont think we would achieve that through a type parameter it would be an overload: function formatDate(date: Date, formatStr: "MMM", options: UseDateFormatOptions = {}): Month;
function formatDate(date: Date, formatStr: string, options: UseDateFormatOptions = {}): string; but that is way off on a tangent for this PR. we should open a separate PR for that, if we even bother (as the only format you can strongly type is the month name, so doesn't seem worth it). what this PR exists for and what the OP originally wanted, i think was a misunderstanding. but lets wait for a response they specifically said |
Before submitting the PR, please make sure you do the following
fixes #123
).Description
Additional context