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

feat(DatePicker): update date library package #761

Merged
merged 48 commits into from
Mar 30, 2024

Conversation

epr3
Copy link
Collaborator

@epr3 epr3 commented Mar 17, 2024

Closes #742.

Hello,

I forked and refactored Adobe's @internationalized/date package and converted all ES6 classes into plain objects and converted the original class methods into functions. All functions return new calendar objects and are immutable.

Then, I've refactored the whole implementation of the Calendar, DateField and DatePicker components to use this library and removed some redundant functions, updated the docs and added localised stories for some of the calendars in Histoire.

It should fix all errors that were caused by Vue changing the this context from the DateValue class to it's own Proxy class (plus any date jumps to the Gregorian Calendar and back causing dates to desync).

This is the library: https://www.npmjs.com/package/flat-internationalized-date

If you guys want, I can relinquish the package ownership to the radix-vue org in Github and continue maintenance under radix-vue.

I'm looking forward to any feedback and improvements you suggest.

Thanks!

CC-ing @sadeghbarati for the Persian Calendar fixes.

@sadeghbarati
Copy link
Contributor

❤️

a Question,

How to use another Calendar System without setting year, month and day manually

before:

const persian = createCalendarDate({
  calendar: CALENDAR.PERSIAN,
  year: 1403,
  month: 12,
  day: 1,
})

after:

const persian = createCalendarDate({
  calendar: CALENDAR.PERSIAN,
})

Automatically use today as the date with related Calendar System

@epr3
Copy link
Collaborator Author

epr3 commented Mar 17, 2024

@sadeghbarati You could try this approach for getting today's date in a different calendar system.

import { today, getLocalTimeZone, toCalendar, CALENDAR } from 'flat-internationalized-date'

const persian = toCalendar(today(getLocalTimeZone()), CALENDAR.PERSIAN)

Copy link
Collaborator

@zernonia zernonia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Phenomenal work @epr3 ! Will have a look asap! 💚

@MellKam
Copy link
Collaborator

MellKam commented Mar 25, 2024

@epr3 Thanks! That will help

@MellKam
Copy link
Collaborator

MellKam commented Mar 26, 2024

I think the problem lies in this function. You need to .copy() the dates before you return them. For me it fixes the story with controlled value.

export function getDefaultDate(props?: GetDefaultDateProps): DateValue {
const withDefaults = { ...defaultDateDefaults, ...props }
const { defaultValue, defaultPlaceholder, granularity } = withDefaults
if (Array.isArray(defaultValue) && defaultValue.length)
return defaultValue[defaultValue.length - 1]
if (defaultValue && !Array.isArray(defaultValue)) {
return defaultValue
}
else if (defaultPlaceholder) {
return defaultPlaceholder
}
else {
const date = new Date()
const year = date.getFullYear()
const month = date.getMonth() + 1
const day = date.getDate()
const calendarDateTimeGranularities = ['hour', 'minute', 'second']
if (calendarDateTimeGranularities.includes(granularity ?? 'day'))
return new CalendarDateTime(year, month, day, 0, 0, 0)
return new CalendarDate(year, month, day)
}
}

@MellKam
Copy link
Collaborator

MellKam commented Mar 26, 2024

MellKam@feb8801

@epr3
Copy link
Collaborator Author

epr3 commented Mar 26, 2024

@MellKam I pulled in your repo and set it up, looks like Histoire still outputs errors:

Screen.Recording.2024-03-26.at.11.15.19.mov

But on Stackblitz, it is working properly. 👍🏻

@MellKam
Copy link
Collaborator

MellKam commented Mar 26, 2024

It's strange because it doesn't work for me on the histoire page either, but it works when I open it in a separate window. Maybe it is some problem with the histoire cache

@epr3
Copy link
Collaborator Author

epr3 commented Mar 26, 2024

Different browser, incognito window:

Screen.Recording.2024-03-26.at.11.41.20.mov

Removed node_modules, ran a pnpm i --force, then opened the story in an incognito window:

Screen.Recording.2024-03-26.at.11.45.16.mov

Am I doing something wrong?

@MellKam
Copy link
Collaborator

MellKam commented Mar 26, 2024

Wait, now it's a different error. Let me check it

@MellKam
Copy link
Collaborator

MellKam commented Mar 26, 2024

All error that we see are related to not found the method in object prototype. For some reason the calendar object can lose it's prototype methods. I dont know at which point this happens. Tried to reproduce it in playground, but without success. Maybe this is actually vue's bug or I just dont see some stupid mistace. Will come back to it this evening.

@epr3
Copy link
Collaborator Author

epr3 commented Mar 26, 2024

One culprit that I could identify (other than Vue's reactivity system) which might cause the DateValue prototype to be reset to a plain object might also be useVModel which does some clones internally and by default it uses JSON.parse(JSON.stringify(value)) to do the clones.

Reference: https://github.com/vueuse/vueuse/blob/1558cd2b5b019abc1feda6d702caa1053a182903/packages/core/useVModel/index.ts#L113

@MellKam
Copy link
Collaborator

MellKam commented Mar 26, 2024

@epr3 That's definitely it! Here is updated version of my previous playground but I changed defineModel to useVModel from @vueuse/core

@epr3
Copy link
Collaborator Author

epr3 commented Mar 26, 2024

I'm overwriting the useVModel values in CalendarRoot with this (to get over JSON.parse):

const modelValue = useVModel(props, 'modelValue', emits, {
  defaultValue: props.defaultValue ?? undefined,
  passive: (props.modelValue === undefined) as false,
  clone: value => Array.isArray(value) ? value.map(item => item.copy()) : (value?.copy() as DateValue),
}) as Ref<DateValue | DateValue[] | undefined>

const defaultDate = getDefaultDate({
  defaultPlaceholder: props.placeholder,
  defaultValue: modelValue.value,
})

const placeholder = useVModel(props, 'placeholder', emits, {
  defaultValue: props.defaultPlaceholder ?? defaultDate.copy(),
  passive: (props.placeholder === undefined) as false,
  clone: value => value?.copy(),
}) as Ref<DateValue>

But it seems that DateValue loses its prototype still:

Screen.Recording.2024-03-26.at.19.24.09.mov

EDIT: It seems like everything keeps its prototype up until it gets emitted (emitted value gets its prototype stripped)
image

@epr3
Copy link
Collaborator Author

epr3 commented Mar 27, 2024

@MellKam I think I finally figured out the what might be the issue.

I've pulled in your playground code into a Histoire story for debugging and I found out that while everything might work fine in a normal application (or playground in this instance), Histoire might do some shenanigans (like JSON.parse(JSON.stringify(DateValue)) ) while passing data from the story iframe (or rendered story) back to the v-model variable, thus losing the prototype. Also, setting { iframe: false } on the Story layout doesn't work to maybe circumvent some iframe postMessage serialization.

Video for reference:

Screen.Recording.2024-03-27.at.11.40.40.mov

In short, the issue won't happen in a normal Vue app, it's related to Histoire and Histoire only.

I'll revert the date library back to @internationalized/date, revert all the methods, tests, stories and docs and keep the other fixes on this branch to be merged.

I'll also dig through Histoire and see how it passes data between its Vue apps (app shell <-> story communication) and open an issue + PR there if I find anything amiss and I can fix.

Sorry for the delays in getting these fixes merged.

cc @sadeghbarati Unfortunately, until Adobe merges this PR, the PersianCalendar will output incorrect dates on some months.

@sadeghbarati
Copy link
Contributor

sadeghbarati commented Mar 27, 2024

@epr3, that's OK ❤️ I'll patch it

@epr3
Copy link
Collaborator Author

epr3 commented Mar 27, 2024

@zernonia I've reverted the date library back to @internationalized/date. PR is ready for another review 🚀

cc @MellKam

Copy link
Collaborator

@zernonia zernonia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a quick question, and I'm thoroughly amazed by all the discussion, research and back-and-forth testing to nail down the issue here!

Really apprecate @epr3 @sadeghbarati @MellKam !!! 💚💯

@zernonia zernonia merged commit 2c86a7c into radix-vue:main Mar 30, 2024
2 of 3 checks passed
@epr3 epr3 deleted the 742-calendar-systems branch April 10, 2024 14:58
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.

[Feature]: Support other Calendar Systems
4 participants