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

startOf('week') is not respecting locale setting #373

Closed
mars-lan opened this issue Nov 22, 2018 · 28 comments
Closed

startOf('week') is not respecting locale setting #373

mars-lan opened this issue Nov 22, 2018 · 28 comments

Comments

@mars-lan
Copy link

While most countries in the world assume the week starts on Monday, US, Canada, and Australia consider Sunday as the first day of the week. It seems that luxon hard-coded the start of week as Monday.

// prints "2018-11-19T00:00:00.000-08:00"
console.log(DateTime.fromISO('2018-11-22T10:00', {zone: 'America/Los_Angeles'}.startOf('week').toISO());

Note that moment does actually respect the locale setting

// prints "2018-11-18T08:00:00.000Z"
console.log(moment.tz('2018-11-22T10:00', "America/Los_Angeles").startOf('week').toISOString());
@icambron
Copy link
Member

Correct. Luxon has no sense of local week start conventions and just uses ISO rules. As far as I know, there's no Intl API I can use to get the local first day of the week.

@dantman
Copy link

dantman commented Oct 1, 2019

Can we put this information in the documentation? I scoured it looking for a setting to set what day of the week is considered the start or how luxon decides what the start of the week is and hence what the basis would be for any offset I'd need to control it.

@icambron
Copy link
Member

icambron commented Oct 2, 2019

It is documented in the weekday accessor: https://moment.github.io/luxon/docs/class/src/datetime.js~DateTime.html#instance-get-weekday

I suppose we could add that documentation in the startOf and endOf docs?

@dantman
Copy link

dantman commented Oct 2, 2019

Yeah, I would have never thought to look at .weekday for the start of the week.

@gregorskii
Copy link

Hi would it make sense to have a setter for this? We are running into this issue now where we are grouping data by weeks and we are finding the result is incorrect as our front end respects locale, and our backend is using Luxon and does not.

Thanks.

@icambron
Copy link
Member

icambron commented Apr 8, 2020

@gregorskii Local week sounds like a great plugin. It is not something I can reasonably support in the core library, because it would work completely differently from all the other local settings. If this ever moves forward, I will add it to Luxon.

@gregorskii
Copy link

Would it be possible to have a setter, or is it too integral to how it works internally?

@icambron
Copy link
Member

icambron commented Apr 8, 2020

Meaning a setter for which day of the week is first? No, I don't want to do that. It's not so much about the internals as the API. All the other features work automatically using the locale settings. To have this one other one where the contract is that first you set some setting and then it works, and if you change locales you need to set again, etc, is surprising departure for the API. That's why I'd like it to be a plugin of some kind.

@gregorskii
Copy link

Makes sense. By plugin do you mean based on the locale? Or does Luxon have the intention of adding plugins?

@wkeese
Copy link

wkeese commented Apr 12, 2020

I just ran across this limitation too. I implemented my own function to do it in https://gist.github.com/wkeese/20514beb68bc1bac807ec07cda4175db.

Note that dojo's data came from the CLDR but we just inlined it into the function.

Also, here's the test case I used:

$ npm install full-icu
$ node --icu-data-dir=./node_modules/full-icu
> const { DateTime } = require("luxon");
undefined
> DateTime.fromObject({year: 2020, month: 4, day: 12, locale: "en-us"}).startOf("week").day
6
> DateTime.fromObject({year: 2020, month: 4, day: 12, locale: "bn-BD"}).startOf("week").day
6

Before reading this ticket, I expected the en-us call to return Sunday, April 5, 2020 and the bn-BD one to return Friday, April 10, 2020.

@Nosfistis
Copy link

I also have a case where I need to display periods according to the week number (e.g. 2020W27 is 28/6/2020 - 4/7/2020) but consider Sunday as the start of the week, no matter the user's locale.

I think we should be able to configure this as a global setting, same as what is considered the first week of the year. Moment.js supports it (https://momentjs.com/docs/#/customization/dow-doy/).

@dasa
Copy link
Contributor

dasa commented Jul 22, 2021

As far as I know, there's no Intl API I can use to get the local first day of the week.

It's coming soon: https://chromestatus.com/feature/5566859262820352

namjul pushed a commit to dendronhq/dendron that referenced this issue Aug 2, 2021
Currently luxon does not support setting first day of the week (moment/luxon#373) therefore indefinitely removing it.
namjul pushed a commit to dendronhq/dendron that referenced this issue Aug 2, 2021
Currently luxon does not support setting first day of the week (moment/luxon#373) therefore indefinitely removing it.
kevinslin pushed a commit to dendronhq/dendron that referenced this issue Aug 6, 2021
* spike: add luxon based config generator

- The luxon mapping was copied from react-component/picker#230

* spike: fix format string difference

Luxon behaves different with format strings.

```tsx
const l = DateTime.fromFormat("2021.07", "y.MM.dd") // results in an invalid luxon date
l.toFormat("y.MM.dd") // Invalid DateTime
const m = moment("2021.07", "y.MM.dd"); // results in an valid moment date
m.format("y.MM.DD") // "2021.07.01"
```

* spike: add default value

It might not be functional necessary but it should improve readablity.

* spike: use correct luxon format token

In luxon `DD` means localized date with abbreviated month and `dd` day of the month, padded to 2.

* spike: correct naming

* spike: remove `firstDayOfWeek` code

Currently luxon does not support setting first day of the week (moment/luxon#373) therefore indefinitely removing it.

* spike: use `Time` from `@dendronhq/common-all` package

`Time` uses `luxon`.

* spike(calendar-view): inline defaults

There is a case where `config` is `undefined` and then throws at
`getMaybeDatePortion`. Until that is fixed we assume default values locally even though they should be defined only in `packages/engine-server/src/config.ts`.

* spike: remove `moment` import

* spike: fix typescript errors

This removes code from the originator. The part that mappes
`getShortWeekDays` to starting with Sunday actually rendered a week
starting with Monday. Now, with the code removed, it starts with
Tuesday. An issue I still need to fix.

* spike: move non-page file outside `pages/vscode`

nextjs interprets files inside `pages` as actual web pages which
`luxon.ts` is not.

* spike: display correct weekdays

Luxon starts week with Monday indexed as 1.
We need to shift so that 1 points to Monday and not Tuesday

* chore: fix typo
kevinslin pushed a commit to dendronhq/dendron that referenced this issue Aug 8, 2021
* spike: add luxon based config generator

- The luxon mapping was copied from react-component/picker#230

* spike: fix format string difference

Luxon behaves different with format strings.

```tsx
const l = DateTime.fromFormat("2021.07", "y.MM.dd") // results in an invalid luxon date
l.toFormat("y.MM.dd") // Invalid DateTime
const m = moment("2021.07", "y.MM.dd"); // results in an valid moment date
m.format("y.MM.DD") // "2021.07.01"
```

* spike: add default value

It might not be functional necessary but it should improve readablity.

* spike: use correct luxon format token

In luxon `DD` means localized date with abbreviated month and `dd` day of the month, padded to 2.

* spike: correct naming

* spike: remove `firstDayOfWeek` code

Currently luxon does not support setting first day of the week (moment/luxon#373) therefore indefinitely removing it.

* spike: use `Time` from `@dendronhq/common-all` package

`Time` uses `luxon`.

* spike(calendar-view): inline defaults

There is a case where `config` is `undefined` and then throws at
`getMaybeDatePortion`. Until that is fixed we assume default values locally even though they should be defined only in `packages/engine-server/src/config.ts`.

* spike: remove `moment` import

* spike: fix typescript errors

This removes code from the originator. The part that mappes
`getShortWeekDays` to starting with Sunday actually rendered a week
starting with Monday. Now, with the code removed, it starts with
Tuesday. An issue I still need to fix.

* spike: move non-page file outside `pages/vscode`

nextjs interprets files inside `pages` as actual web pages which
`luxon.ts` is not.

* spike: display correct weekdays

Luxon starts week with Monday indexed as 1.
We need to shift so that 1 points to Monday and not Tuesday

* chore: fix typo
@lostdesign
Copy link

lostdesign commented Sep 16, 2021

While this is a quite old issue, I still wanted to share my quick work around:

const date = DateTime.fromISO('2021-09-16').startOf('week')
const sunday = date.minus({day: 1})

By simply subtracting a day, you get a sunday which you can use as a start point for further actions.

edit: However, once possible, this should be part of the API.

@bvandercar-vt
Copy link

@lostdesign this is a misleading solution as it would not work when the day is Sunday. If the current day is Sunday, you would want the start of the week day to be today, but startOf('week') would give you last Monday so your resulting Sunday would be last Sunday, not the current day (Sunday)

@bvandercar-vt
Copy link

@lostdesign proposed solution is this:

const now = DateTime.now()
const sundayStartOfWeek = now
        .startOf('week')
        .plus({ week: now.weekdayShort === 'Sun' ? 1 : 0 })
        .minus({ day: 1 })

@lostdesign
Copy link

lostdesign commented Dec 7, 2021

Not quite sure what you mean @bvandercar-vt - your solution acts just the same. The solution proposed will always get the previous sunday which just works as I proposed.

Bildschirmfoto 2021-12-07 um 09 52 06


If someone needs the current weeks sunday, they should use the solution below.

import { DateTime } from 'luxon'

// using the 5th of december which is a sunday
const date = DateTime.fromISO('2021-12-05').setLocale('en-US')

const getSunday = 
  date.weekdayShort === 'Sun' 
    ? date.toISODate()
    : date.startOf('week').minus({day: 1}).toISODate()

// will return the 5th of december
getSunday

Bildschirmfoto 2021-12-07 um 09 56 55

This again is a work around, as you get the current sunday and can build your own week from that date on to create your sunday-as-first day week, iterating over the date and pushing the following 6 days into an array. I once wrote a method to get the current week dates based of a date that is passed.

https://github.com/lostdesign/linked/blob/58c8796d2059e26572366b846dd26212b789ea97/src/store/modules/calendar/helper.js#L38-L56

@iknowmac
Copy link

In my case, I need to get the start of the week given any day of the week...

const getStartOfWeek(dateTime: DateTimeType) = {
    return dateTime.minus({ days: dateTime.weekday }).startOf('day');
}

Cheers...

@lostdesign
Copy link

In my case, I need to get the start of the week given any day of the week...

const getStartOfWeek(dateTime: DateTimeType) = {
    return dateTime.minus({ days: dateTime.weekday }).startOf('day');
}

Cheers...

The topic is especially about the locale-based start of week..

@sam-s4s
Copy link

sam-s4s commented May 3, 2022

I'd just like to chime in here, and say that in the original post, @mars-lan has stated that Sunday is the start of week for Australia. This is incorrect.

Australia adheres to ISO 8601-2007 (and in turn, ISO 8601) - which clearly states Monday is the first day of the week.

This can be verified in many places, but here is the wikipedia for easy reading:
https://en.wikipedia.org/wiki/ISO_8601
Note the inclusion of Australia under the "Adoption as national standards" section.

And the definition "[D] is the weekday number, from 1 through 7, beginning with Monday and ending with Sunday."

For a brief period, in the official Unicode Common Locale Data Repository (CLDR), Australia's start day was incorrectly changed to Sunday due to a misunderstanding. But after the following case, it was changed back to Monday, which is correct.

https://unicode-org.atlassian.net/browse/CLDR-14795

So hopefully Luxon does correctly use Monday as the start of the week for Australia.

@FirefighterBlu3
Copy link

( .weekday%7) for USA/Canada. this will map to [Su Mo Tu We Th Fr Sa], +1 if you are using a grid of 1-7 instead of 0-6

@lucasgladding
Copy link

lucasgladding commented Jul 7, 2022

In case anyone else is experimenting with this, this is what I am using:

function getWeekStart(date) {
  const offset = Duration.fromObject({ days: 1 })
  return date
    .plus(offset)
    .startOf('week')
    .minus(offset)
}

To account for cases where the reference date is Sunday, the reference date is increased by one, the start is calculated, then that date is decreased. I haven't encountered any issues with the approach so far.

@dilungasr
Copy link

In my case, I need to get the start of the week given any day of the week...

const getStartOfWeek(dateTime: DateTimeType) = {
    return dateTime.minus({ days: dateTime.weekday }).startOf('day');
}

Cheers...

You nailed it!

@FawzyMokhtar
Copy link

The start day of the week in Arab countries is Saturday, so that I came with the following workaround:

  1. Extend the DateTime type to add more utilities [index.d.ts]:

     import { DateTime } from 'luxon';
    
     declare module 'luxon/src/datetime' {
       export interface DateTime {
         /**
          * Returns new `DateTime` instance with the time sets to the first day in the current week.
          */
          startOfWeek(): DateTime;
    
         /**
          * Returns new `DateTime` instance with the time sets to the last day in the current week.
          */
          endOfWeek(): DateTime;
    
         /**
          * Returns new `DateTime` instance with the time sets to the first day in the current Arabic week.
          */
          startOfArabicWeek(): DateTime;
    
         /**
          * Returns new `DateTime` instance with the time sets to the last day in the current Arabic week.
          */
          endOfArabicWeek(): DateTime;
       }
     }
  2. Add the implementation of the new methods [app.ts]:

     DateTime.prototype.startOfWeek = function (): DateTime {
       return this.startOf('week');
     };
    
     DateTime.prototype.endOfWeek = function (): DateTime {
       return this.endOf('week');
     };
    
     DateTime.prototype.startOfArabicWeek = function (): DateTime {
       /**
        * Because the start of week in the luxon package is always Monday.
        * We are going back two days to reach Saturday.
        */
       return this.startOf('week').minus({ days: 2 });
     };
    
     DateTime.prototype.endOfArabicWeek = function (): DateTime {
       /**
        * Because the end of week in the luxon package is always Sunday.
        * We are going back two days to reach Friday.
        */
       return this.endOf('week').minus({ days: 2 });
     };

@Nathan-Lynx
Copy link

Nathan-Lynx commented Aug 6, 2023

While this is a quite old issue, I still wanted to share my quick work around:

const date = DateTime.fromISO('2021-09-16').startOf('week')
const sunday = date.minus({day: 1})

By simply subtracting a day, you get a sunday which you can use as a start point for further actions.

edit: However, once possible, this should be part of the API.


Don't run that code on Sundays

@cluxig
Copy link

cluxig commented Nov 23, 2023

I came to the following overlay, which can be imported as patch-file for DateTime.
Remember to specify your necessary locales within weekStartMap. This map is based on the fixed, locale-non-aware weekdays of luxon (1=Monday, 7=Sunday).

Tip: To import this file, e.g. within angular project, just add import 'path/to/datetime.patch'; in your main.ts
Afterwards you can just go with DateTime.fromISO(...).startOfWeek().

import { DateTime } from 'luxon';

const weekStartMap: Record<string, number> = {
    'en-US': 7,
    'en-CA': 7,
    // add your necessary locations, default is monday-based (1)
};

function posMod(n: number, m: number): number {
	return ((n % m) + m) % m;
}

declare module 'luxon' {
	interface DateTime {
		/**
		 * Since Luxon's startOf('week') is always Monday (weekday=1), this method is added to jump to the start of the week by respecting the DateTime's locale.
		 * Assure, that weekStartMap contains the locale's first day of week in range 1-7 (Mo-Su).
		 * The default first day of week is Monday (1).
		 */
		startOfWeek(): DateTime;
		/**
		 * Since Luxon's endOf('week') is always Sunday (weekday=7), this method is added to jump to the end of the week by respecting the DateTime's locale.
		 * Assure, that weekStartMap contains the locale's first day of week in range 1-7 (Mo-Su).
		 * The default first day of week is Monday (1).
		 */
		endOfWeek(): DateTime;
	}
}

DateTime.prototype.startOfWeek = function (): DateTime {
	if (this.isValid) {
		const firstDayOfWeek = this.locale ? weekStartMap[this.locale] ?? 1 : 1;
		const diff = posMod(this.weekday - firstDayOfWeek, 7);
		return this.startOf('day').minus({ days: diff });
	} else {
		return this;
	}
};

DateTime.prototype.endOfWeek = function (): DateTime {
	if (this.isValid) {
		const firstDayOfWeek = this.locale ? weekStartMap[this.locale] ?? 1 : 1;
		const diff = posMod(firstDayOfWeek - this.weekday - 1, 7);
		return this.endOf('day').plus({ days: diff });
	} else {
		return this;
	}
};

@diesieben07
Copy link
Collaborator

Luxon now supports locale-based weeks:
https://moment.github.io/luxon/#/intl?id=locale-based-weeks

@cluxig
Copy link

cluxig commented Nov 24, 2023

Luxon now supports locale-based weeks: https://moment.github.io/luxon/#/intl?id=locale-based-weeks

Thanks for the hint. Unfortunately this is not covered by latest (3.3.5) @types/luxon so far. Additionally my workaround allows Browser-Support before Chrome 99, which was release on 2022-03. Using your hint, I can check if Intl.Locale#getWeekInfo is available at runtime and switch implementation if possible.

@diesieben07
Copy link
Collaborator

If you must support older browsers without native support, I would argue that a polyfill for the native API is a better option.

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