moment / luxon

⏱ A library for working with dates and times in JS
https://moment.github.io/luxon
MIT License
15.23k stars 730 forks source link

Support Intl start of week settings #1447

Closed lawrencety closed 11 months ago

lawrencety commented 1 year ago

Is your feature request related to a problem? Please describe. I've seen several closed issues regarding this problem where there is no way to set the start of week manually or based on the new Intl getWeekInfo method. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl/Locale/getWeekInfo I am working on an international application which will need dynamic switching of startOfWeek for the appropriate audience. We can capture this via the new Intl API, but I could not find anywhere to either control it in luxon or integrating the new functionality into setting locale.

Describe the solution you'd like Setting locale should leverage the Intl getWeekInfo API to set the startOfWeek weekday to the appropriate day.

Describe alternatives you've considered Add a config or setting option to change the startOfWeek to a weekday (0-6)

Additional context Some component libraries do not let devs manually change the start of week, it is based on the datetime library. https://github.com/mui/mui-x/issues/12821

Intl API: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl/Locale/getWeekInfo

diesieben07 commented 1 year ago

I will do some prototyping to figure out how to best integrate it with Luxon's existing APIs regarding week days.

diesieben07 commented 1 year ago

My initial findings are promising. My current quick implementation adds a new unit localeWeek. It behaves exactly the same as week pretty much everywhere, except for startOf, endOf and hasSame (only startOf actually does date computations, the other two just rely on it). When used with those three, it uses the locale's start of week instead. This makes the following work:

const dt = DateTime.fromISO('2023-06-13T09:00:00');

const startDe = dt.reconfigure({locale: 'de-DE'}).startOf('localeWeek');
console.log(startDe.toISO(), startDe.weekdayLong); // 2023-06-12T00:00:00.000+02:00 Montag - in Germany the week starts on Monday

const startEn = dt.reconfigure({locale: 'en-US'}).startOf('localeWeek');
console.log(startEn.toISO(), startEn.weekdayLong); // 2023-06-11T00:00:00.000+02:00 Sunday - in the US the week starts on Sunday

I am currently unsure if I want to keep the "hack" of just adding localeWeek as a unit and special-casing it in startOf. As an alternative I might introduce an options parameter for startOf, endOf and hasSame instead, which allows you to switch to locale-based start of week.

You can find the current implementation here: https://github.com/diesieben07/luxon/commit/a47a7046aa4558f202d996aa71e125a6f0098aad

icambron commented 1 year ago

@diesieben07 I like the option more, I think. If we allow "localeWeek", many users will be confused about which methods accept it in the units slot.

An interesting question is whether we allow local week number as a parsing token too, along with (presumably) some sort of local week year token, and a matching fromLocaleWeek() factory method. Moment allows this, but it leads into some thorny territory: how do week years work? IIRC, Moment actually lets the locale decide the rule for when the week year starts, which I think was probably a mistake (are there really local convention on "week years"? I doubt it, and anyway we presumably don't have a way to get it). Anyway, this is just musing: no reason we have to add this at the same time as the startOf stuff.

diesieben07 commented 1 year ago

Yes, agreed on using an option, it feels cleaner and clearer.

RE parsing and locale week years, the Intl.Locale.weekInfo API at the very least exposes info on this in the form of minimalDays:

An integer between 1 and 7 indicating the minimal days required in the first week of a month or year, for calendar purposes.

I don't know how accurate this actually is and if this value is actually relevant in the real world. but the info is there. It does report different values for DE and US locales (4 vs. 1), so there seems to be some reality to it.

icambron commented 1 year ago

@diesieben07 interesting, I stand corrected. It seems fine to use then

lawrencety commented 1 year ago

would this change require updates from MUI library to query the new "localeWeek" option or would the AdapterLuxon need to be updated to incorporate this change or would this be automatically picked up via their calendar component?

see MUI issue: https://github.com/mui/mui-x/issues/12821

diesieben07 commented 1 year ago

I don't use MUI, but if I understand the concept of "adapter" correctly here, it seems like something that the Luxon adapter would have to do. Likely there will be a new method added to the Info class, something like startOfWeek(locale), which will report on which day of the week the week starts. However this can already be accomplished today, by using Intl.Locale#getWeekInfo directly. The weekday property will most likely always refer to ISO weekdays, starting Monday.

Robinfr commented 1 year ago

I've also been waiting for this for a long time, and it is one of the few reasons we cannot adopt this library at the moment (https://github.com/moment/luxon/issues/1293). If there's anything I can do to help get this into Luxon, I'd be glad to help!

diesieben07 commented 1 year ago

@Robinfr You can see the current status of my implementation in my draft pull request. I hope to finish work on it shortly.

jcompagner commented 1 year ago

the weird thing is for me using Edge (chromium) the week day object that Intl gives is just wrong:

image

US should be 7 right as the first day? (its sunday)

diesieben07 commented 1 year ago

You're looking for en-US.

marjanaet commented 1 year ago

my draft pull request

Any plans on merging this PR 🙁? We are waiting for this

diesieben07 commented 1 year ago

The PR is not finished yet I am afraid. I still need to give it some finishing touches.

svey commented 11 months ago

The PR is not finished yet I am afraid. I still need to give it some finishing touches.

Thank you for working on this 🥳 @diesieben07

carvalheiro commented 10 months ago

Do we have a sense of when this version will be available on npm? @diesieben07

diesieben07 commented 10 months ago

I'm not in charge of releases, you'll have to ask @icambron

carvalheiro commented 10 months ago

@icambron could you tell me when this is release is planned to go out?