Dayjs: expect(dayjs('2019-07-28').week()).toBe(30)

[kirillgroshkov]

Describe the bug
Since the introduction of locale.weekStart I expect it to work with e.g .week() plugin, but currently it doesn't. 2019-07-28 is Sunday, so in en-gb locale it should be part of week 30, not week 31.

In Jest syntax:

require('dayjs/locale/en-gb')
dayjs.locale('en-gb')

expect(dayjs('2019-07-28').week()).toBe(30)

throws:

Expected: 30
Received: 31

Information

• Day.js Version: 1.8.15
• OS: MacOS
• Browser Chrome 76
• Time zone: Stockholm

UPD: attaching a screenshot of my WeekNumberAware calendar:

[kirillgroshkov]

I _almost_ found a solution by changing one line:

// before
const compareDay = startOfYear.subtract(startOfYear.day(), 'd').subtract(1, 'ms')
// after
const compareDay = startOfYear.subtract(startOfYear.day() - weekStart, 'd').subtract(1, 'ms')

// const weekStart = (this as any).$locale().weekStart || 0

Running test for last year and it works, except for 2018-12-30 and 2019-12-29 (last Sunday of the year)...

[kirillgroshkov]

Ahaa! These edge cases fail because of this condition:

if (endOfYear.day() !== 6 && this.month() === 11 && (31 - this.date()) <= endOfYear.day()) {
      return 1
    }

[kirillgroshkov]

Found good solution: https://stackoverflow.com/a/6117889/4919972

Will post a link to my plugin when I'm done...

[kirillgroshkov]

So, basically, the problem is to support 3 popular world "locales": ISO (Europe), US/Canada (weekStart=0) and Arab countries where weekStart=6.

With limited time I couldn't hack together a universal solution, so I ended up there: https://github.com/NaturalCycles/time-lib/blob/master/src/plugin/weekOfYear.ts

I'm taking US/Canada solution from stock Dayjs weekOfYear plugin (it works ONLY for US/Canada, but breaks for other 2 cases). Stackoverflow solution works for ISO (but breaks for other 2). I've adopted both solutions with simple if (weekStart =0) ... else .... But no solution for weekStart=6 yet.

I'm happy if someone can fix a proper solution for all these 3 cases.

I'm surprised how hard of a problem this can be...

[gpbl]

@kirillgroshkov I'm running into the same problem. I wonder if taking a look to how moment.js (or other libraries) have implemented it should help: https://github.com/moment/moment/blob/96d0d6791ab495859d09a868803d31a55c917de1/moment.js#L1212

[g1eny0ung]

I will be working on this. Thanks for finding this problem.

[gpbl]

@g1eny0ung that would be great! I'm evaluating this library for the next version of https://github.com/gpbl/react-day-picker and this is so far the only blocker.

[g1eny0ung]

@kirillgroshkov @gpbl Sorry for the delay. I'm a little busy in daily works.

I test the weekOfYear plugin in https://runkit.com/g1eny0ung/5d4b84fed30292001ad9ba08

Because the weekOfYear plugin didn't consider the situation of locale, the result of dayjs('2019-07-28').week() is same as moment('2019-07-28').week().

But if we set locale in momentjs, like moment.locale('en-gb'), then momentjs will consider the locale in the result of a week of the year.

I think the solution mentioned above is correct:

const compareDay = startOfYear.subtract(startOfYear.day() - weekStart, 'd').subtract(1, 'ms')

@iamkun What do you think?

If there is nothing wrong with it, I will open a PR to solve this issue.

[g1eny0ung]

Progress at PR #658.

[kirillgroshkov]

@kirillgroshkov @gpbl Sorry for the delay. I'm a little busy in daily works.

I test the weekOfYear plugin in https://runkit.com/g1eny0ung/5d4b84fed30292001ad9ba08

Because the weekOfYear plugin didn't consider the situation of locale, the result of dayjs('2019-07-28').week() is same as moment('2019-07-28').week().

But if we set locale in momentjs, like moment.locale('en-gb'), then momentjs will consider the locale in the result of a week of the year.

I think the solution mentioned above is correct:

const compareDay = startOfYear.subtract(startOfYear.day() - weekStart, 'd').subtract(1, 'ms')

But it doesn't work for weekStart=6 (Arabic countries)

[kirillgroshkov]

Also, for proper testing - loop over few years of days and compare momentjs and dayjs. There are edge cases around year start that I didn't manage to fix.

Example of how I tested it: https://github.com/NaturalCycles/time-lib/blob/master/src/plugin/weekOfYear.test.ts

[g1eny0ung]

Thanks for your suggestion. I will check it again. 👍🏽

_{Sent with GitHawk}

[g1eny0ung]

I have been researching for a long time yesterday.

TL;DR

Now we have not yet decided how to achieve week with locale. The current week() calculation method is flawed.

Details

According to https://en.wikipedia.org/wiki/ISO_8601#Week_dates, it's easy to calculate the ISO week of the year with two constraints:

The one is which day is the week start, for example, the ISO agreed that the first day of the week is Monday.

The first week of the year is the week that contains that year's first Thursday.

The Implementation can refer to https://www.epochconverter.com/weeknumbers

Also, I have researched the related source code of moment.js, in:

https://github.com/moment/moment/blob/develop/src/lib/units/week-calendar-utils.js#L39

moment.js calculate also with two constraints, dow and doy, day of week and day of the year.

For example, in locale ar:

https://github.com/moment/moment/blob/develop/src/locale/ar.js#L124

{
  week : {
    dow : 6, // Saturday is the first day of the week.
    doy : 12  // The week that contains Jan 12th is the first week of the year.
  }
}

The problem in day.js weekOfyear plugin is, we didn't use any of the methods mentioned above. We just calculate the distance from the first day of the year to the present:

So, it's incorrect. Even we add weekStart condition in PR #658, it's also incorrect. Because it's wrong at first.

I have already discussed with @iamkun, currently, we haven't thought about how to re-implement this plugin. Copy from moment.js? Discard some features (only support ISO to make day.js still tiny and fast)?

We haven't thought about it yet.

So we decide to pending this issue, merge the PR (at least it handles some of the current errors).

If we have new progress in this plugin, we will comment here to notify everybody. Thx everyone for their contribution.

[iamkun]

:tada: This issue has been resolved in version 1.8.16 :tada:

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot :package::rocket: