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

[Feature]: Replace DateValue type with generic in calenar/date-picker component #1641

Open
2 tasks
mkeyy0 opened this issue Feb 20, 2025 · 18 comments
Open
2 tasks

Comments

@mkeyy0
Copy link

mkeyy0 commented Feb 20, 2025

Describe the feature

I'm currently adding a Calendar component to my project and I found it a bit annoying to work with the Calendar component from a type perspective, as the default type for the modelValue is DateValue you always have to cast your type to DateValue like this:

const date = ref(new CalendarDate(2025, 1, 1)) as Ref<DateValue>
<template>
  <Calendar v-model="date" />
</template>

And as other props and emits are typed as DateValue you always have to write as or some people may write the actual if-check in functions like nextPage, previousPage update:modelValue etc. And it's not great from a DX perspective too. I would suggest adding a generic instead that by default fallback to the CalendarDate type and in case a user defined it as a CalendarDateTime or ZonedDateTime it should infer other props and emit types based on this.

From my perspective, it might be a good addition to v2 and will simplify working with date-related components and make types more narrow

If some help or other info is needed from me I would be happy to assit

Additional information

  • I intend to submit a PR for this feature.
  • I have already implemented and/or tested this feature.
@mkeyy0
Copy link
Author

mkeyy0 commented Feb 21, 2025

Also, it looks like generic doesn't solve the issue because the @internationalized/date package uses ESM private fields in the classes it exposes, and vue, when unwraps ref, can't unwrap private fields typings. But even more concerning is the fact that this issue mentioned that you can't use a proxy with native # private fields because it might cause a runtime error and suggests using markRaw with ref with these kinds of classes. Also in this case you wouldn't need to use the as operator every time you use any date-related components.
vuejs/core#2981
vuejs/core#7240

But maybe it's old information and something has changed since then.

@Solant
Copy link

Solant commented Mar 3, 2025

Having the same issues when trying to pass CalendarDate instance between components

@Solant
Copy link

Solant commented Mar 4, 2025

@mkeyy0 from what I've checked, private fields of CalendarDate are implemented via swc polyfill function _class_private_field_init, I think this is the reason we don't have runtime errors yet

Image

@mkeyy0
Copy link
Author

mkeyy0 commented Mar 4, 2025

Yeah, I personally ended up using different calendar library, while radix-vue calendar is good, I don't want to use @internationalized/date that relies on classes because if they potentially remove polyfil for private class field it might cause runtime issues. And from type-perspective too, it's not ideal writing type assertions every time you use calendar. (But it might be solved by component wrapper, that exclude private field from generic constraint)

@cyyynthia
Copy link
Contributor

Using markRaw when passing date objects is indeed the correct thing to do, and TypeScript is correctly reporting a type error here. It's the big caveat of proxies, that when private properties are involved it can no longer be treated as transparent.

It's quite annoying that it has to be done this way, but I don't think this is something Reka can solve by itself, except perhaps providing overloaded objects that apply markRaw in the constructor, but this actually means also shimming all the functions exposed by the lib... 😕

@Solant
Copy link

Solant commented Mar 5, 2025

Using markRaw when passing date objects is indeed the correct thing to do

Aren't we disabling reactivity with this? I would really like to change date objects using Calendar occasionally 😀

Maybe we can use shallowRef for this purpose?

@mkeyy0
Copy link
Author

mkeyy0 commented Mar 5, 2025

Yeah, you're disabling reactivity. Still, for most of the cases, it wouldn't be the problem as v-model even now emits new instance of the CalendarDate each time it changes, but yes it is the problem if you modify the CalendarDate using some of its method. Anyway, it looks like a restriction on the reactivity side, but I guess it is at least worth mentioning in the docs that if you want to use date picker you have to use markRaw

@cyyynthia
Copy link
Contributor

cyyynthia commented Mar 5, 2025

Objects from the date lib are immutable in practice, so disabling interior reactivity has no observable effect. All methods return a new instance, never mutate.

ref(markRaw(new CalendarDate(...)))

Shallow ref can do the trick for this specific case, not if you want reactivity and have a date object in structured data

Mentioning it in the doc is a good call

@cyyynthia
Copy link
Contributor

cyyynthia commented Mar 6, 2025

I was able to come up with the following snippet, which makes all date objects behave as marked raw, both at runtime and within the type system.

import type { Raw } from 'vue'

import { CalendarDate, CalendarDateTime, Time, ZonedDateTime } from '@internationalized/date'
import { markRaw } from 'vue'

markRaw(CalendarDate.prototype)
markRaw(Time.prototype)
markRaw(CalendarDateTime.prototype)
markRaw(ZonedDateTime.prototype)

declare module '@internationalized/date' {
  type RawBrand = keyof Raw<{}>
  const RawSymbol: RawBrand

  export interface CalendarDate {
    [RawSymbol]?: true
  }

  export interface Time {
    [RawSymbol]?: true
  }

  export interface CalendarDateTime {
    [RawSymbol]?: true
  }

  export interface ZonedDateTime {
    [RawSymbol]?: true
  }
}

@mkeyy0
Copy link
Author

mkeyy0 commented Mar 13, 2025

In any way, I guess it makes sense to add a generic to the Calendar component. It will make types clearer and stricter. For example, if we provide defaultValue as CalendarDateTime, the rest of our props will also have the CalendarDateTime type. And the same with CalendarDate and ZonedDateTime. Right now the actual value is resolved based on 3 props:

  • modelValue;
  • defaultValue;
  • placeholder.
    I suggest to add this on the time level. It could be done in the following way:
type Props<T extends DateValue = CalendarDate> = {
  modelValue: T;
  defaultValue: T;
  placeholder: T;
  minDate: NoInfer<T>; // no infer as this value doesn't affect actual type of the value
  // ...rest
}
type Emits<T extends DateValue = CalendarDate> = {
  'update:modelValue': [event: T]
}

What do you think?

@Solant
Copy link

Solant commented Mar 13, 2025

I think it will add a lot of uncertainty to the component.

For example:

  1. How should a calendar know its T type in the runtime?
  2. What is the default date part of DateTime?
  3. What is the timezone of ZonedDateTime?
  4. Should we be able to override the default date and timezone?

It is much easier to create an intermediate computed value that transforms v-model value of CalendarDate to whatever suits your needs

@mkeyy0
Copy link
Author

mkeyy0 commented Mar 13, 2025

I'm not sure it adds any uncertainty to the component. To be honest, I don't quite understand how your points are related to the types I proposed.
When you add a generic, you know with which instance you work, while the DateValue is quite generic and doesn't know. You can safely call a specific method that adds overhead, to be honest. You have to write as of even worse if, but with generic it becomes cleaner.

<script setup lang="ts>
const modelValue = shallowRef<CalendarDateTime>();

modelValue.value = new CalendarDateTime(2025, 1, 1, 0, 0, 0);

const someCopy = shallowRef<CalendarDateTime>();
</script>

<template>
  <!-- Can't be safely done without generic types -->
  <Calendar v-model="modelValue" @update:model-value="someCopy = $event" />
</template>

But maybe you don't need minDate: NoInfer<T> this in your type declarations because minDate and other similar props don't affect runtime behavior and any DateValue can be passed there, but for the types like modelValue placeholder and defaultValue I still think that it makes sense to use generic. As it makes types closer to reality and not as wide as they are right now.

@Solant
Copy link

Solant commented Mar 13, 2025

When you add a generic, you know with which instance you work

From the application code — yes. But not from a component perspective. Calendar must know what type it should emit, and for generic like this, it will have to use a lot of instanceof checks to find out the expected type. Even with all of those checks it won't be able to get it:

<script setup lang="ts">
const modelValue = shallowRef<CalendarDateTime>();;
</script>

<template>
  <Calendar v-model="modelValue" />
</template>

By default, modelValue is undefined, and there is no way for the Calendar to know what type it should emit the first time the user selects the date, as Calendar component receives no runtime information about CalendarDateTime being used there

@mkeyy0
Copy link
Author

mkeyy0 commented Mar 13, 2025

Yeah, makes sense. In this case CalendarDate will be emitted while user expect CalendarDateTime

@cyyynthia
Copy link
Contributor

Calendar must know what type it should emit, and for generic like this, it will have to use a lot of instanceof checks to find out the expected type

If it didn't receive any prop, then the type will default to CalendarDate and it can create instances of that. If a value is provided, then it is trivial to only work with derivatives of base objects (.add, .set, ...) which a) won't require knowing the precise type ahead of time (the interface is sufficient) and b) will keep properties intact such as timezone of a ZonedDateTime.

While it's convenient to work with "whatever object type we want", the argument could be made that it is more correct to only work with CalendarDate to avoid any risk w.r.t. properties that the component is not aware of. After all, it is ambiguous what should the calendar do in the situation where it receives a ZonedDateTime; keep the time/tz? reset? from which object if both model and placeholder are provided?

The latter is more constraining especially for v-model, but could be dealt with by using a writable computed or VueUse synchronized refs doing the conversion in both directions. Could even be a Reka-provided helper.

@mkeyy0
Copy link
Author

mkeyy0 commented Mar 14, 2025

Yeah, also, the example I provided is far from ideal because it's not how you use the calendar if you want date time to be honest. Because if you want the v-model to be CalendarDateTime or ZonedDateTime, you have to provide an actual value for defaultValue, placeholder, or modelValue. If you provide a modelValue with incorrect type declaration, it's the code problem because you can write shallowRef() as Ref<CalendarDateTime> with current types. The result will be the same. So usually, type will be inferred based on the actual value, e.g.

<script setup lang="ts">
const modelValue = shallowRef<ZonedDateTime>();

const placeholder = shallowRef(new CalendarDateTime(2025, 1, 1, 0, 0, 0));
</script>

<template>
   <!-- Type error in placeholder, while in the current implementation no type errors -->
  <Calendar v-model="modelValue" :placeholder="placeholder" />
</template>
<script setup lang="ts">
const modelValue = shallowRef<CalendarDateTime>();

const placeholder = shallowRef(new CalendarDateTime(2025, 1, 1, 0, 0, 0));
</script>

<template>
   <!-- no errors -->
  <Calendar v-model="modelValue" :placeholder="placeholder" />
</template>

@mkeyy0
Copy link
Author

mkeyy0 commented Mar 14, 2025

Also, could you explain what problems with ZonedDateTime and CalendarDateTime you are talking about? Cause I've scratched only the surface of the code, I'm not quite sure. For me, everything is straightforward: just use the instance provided by props, and that's it. Why do you even need to modify the timezone or date-time in the calendar component before emitting it back?

  • When you pass the CalendarDateTime - use only date to show the day in the calendar
  • When you pass ZonedDateTime - use the date to show it in the calendar.
    Or am I missing something?

@mkeyy0
Copy link
Author

mkeyy0 commented Mar 14, 2025

But anyway, I'd like to mention that there's a case when multiple is set to true, and in this case, the modelValue has to be an array. From a props perspective, it's quite obvious to just write a discriminated union, but when we want to type emits, it's not that obvious because we have to write conditional types there, and I'm not sure that vue allows us to do that in SFC.

I wrote something that can be potentially a solution, but it doesn't look good, but it works locally for me:

<script
  lang="ts"
  setup
  generic="TDate extends DateValue = CalendarDate, TMultiple extends boolean = false"
>

type Props = {
  multiple?: TMultiple;
  modelValue?: TMultiple extends true ? TDate[] | undefined : TDate | undefined;
  placeholder?: TDate;
} & { class?: HTMLAttributes['class'] };

const props = defineProps<Props & { class?: HTMLAttributes['class'] }>();

const emits = defineEmits<{
  /** Event handler called whenever the model value changes */
  'update:modelValue': [event: TMultiple extends true ? TDate[] | undefined : TDate | undefined];
  /** Event handler called whenever the placeholder value changes */
  'update:placeholder': [event: TDate];
}>();
</script>

With these types, when multiple is set to true, it infers the modelValue as an array of whatever was passed to the props or v-model. If multiple is not provided, then it infers the modelValue and the rest of the inferable values in the same way as in the example I sent above

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