Fix startOf function for isoWeek

[stockiNail]

Fixes #22

[benmccann]

This could probably use some tests

[stockiNail]

This could probably use some tests

Well, as you have seen, I'm not so expert on JS therefore I need time to understand how to do it. Be also aware that I don't have the possibility to tests it on my laptop (probably I don't have the environment ready for that). Locally, I've tested, of course.

Do you mean to add some tests into https://github.com/chartjs/chartjs-adapter-luxon/blob/master/test/specs/luxon-adapter.spec.js?

[benmccann]

Yeah, add a test in that file and then run gulp test-unit to see if it passes. (You'll have to run npm install once to install all the test dependencies but only once to set things up and not each time you run the tests)

I was thinking it'd be good to check each day of the week or at least the ones on the border like Sunday and Monday

Let me know if you have any other questions

[benmccann]

It looks to me like startOf('week') does the iso week in Luxon and there isn't a non-iso option. If that's the case then this could all be simplified to .startOf('week').plus({days: weekday - 1}) (that logic would be simple enough I'd be okay without adding a test, but you should probably manually verify it does what's expected)

[stockiNail]

It doesn't work with your suggestion.

If you try with 2020-08-01 (Saturday, 1st Aug), the startOf (passing 7, Sunday) should be Jul 26th but instead the result is Aug 2nd. If you try my code, it works.

[stockiNail]

Yeah, add a test in that file and then run gulp test-unit to see if it passes. (You'll have to run npm install once to install all the test dependencies but only once to set things up and not each time you run the tests)

I was thinking it'd be good to check each day of the week or at least the ones on the border like Sunday and Monday

Let me know if you have any other questions

Thank you @benmccann

Let me take time but I'm gonna do. My doubt is to install something that I have to use only for this use case.... I don't usually use NodeJS and then npm as well.

[benmccann]

If you try with 2020-08-01 (Saturday, 1st Aug), the startOf (passing 7, Sunday) should be Jul 26th

Why is that? It doesn't sound right to me. The start of the week is July 27 (Monday). The 7th day of that week is Aug 2 (Sunday)

I just tried moment("2020-08-01").isoWeekday(7); and got Aug 2: https://plnkr.co/edit/i6q8LnRQP4oJHWqP?open=lib%2Fscript.js

[stockiNail]

Maybe I'm wrong to understand what weekday is. For me weekday is the day that I want to be the first day of my week, overriding the 1 (Monday) usually used. If it does not have this meaning, I think the weekday is quite useless and .startOf('week') then should be enough. Again, maybe I'm wrong.

[stockiNail]

@benmccann I have created the unit test, checking all days of August and it works with gulp (not committed yet).

it('should startOf correctly using isoWeek preset', function() {
    const adapter = new _adapters._date();
    const dayOfWeekNames = ['Mon', 'Tue', 'Wed', 'Thu', 'Fri', 'Sat', 'Sun'];

    for (let dayOfWeek=0; dayOfWeek<dayOfWeekNames.length; dayOfWeek++) {
        for (let dayOfMonth=1; dayOfMonth<=31; dayOfMonth++){
            const dt = DateTime.local(2020, 8, dayOfMonth, 8, 30);
            const startOf = adapter.startOf(dt.valueOf(), 'isoWeek', dayOfWeek+1);
            expect(adapter.format(startOf, 'ccc')).toEqual(dayOfWeekNames[dayOfWeek]);
        }
    }
});

[benmccann]

Ah, no, weekday isn't the day that the week starts on. It's the day of the week to use with 1 being the first day of the week and 7 being the last day of the week

[stockiNail]

It's the day of the week to use with 1 being the first day of the week and 7 being the last day of the week

I'm completely lost, sorry. Let me try to do an example:

startOf ("2020-08-01", "isoWeek", 7): the result should be 2020-08-02 because I'm requesting the last day of the week of that date. Is it right?

Edit: if it is so, .startOf('week').plus({days: weekday - 1}) is correct.

[kurkle]

Date-fns adapter (and I think moment too) take the parameter as the start day of week. So I think stokiNail is correct.

[benmccann]

The moment adapter does time.isoWeekday(weekday).valueOf() https://github.com/chartjs/chartjs-adapter-moment/blob/e86d579262bc8a9e50f01ad85c89e95dff7b81e3/src/index.js#L47

And moment("2020-08-01").isoWeekday(7); returns Aug 2. Maybe I don't have the right terminology, but I'd expect that to be the result here too, unless I'm missing something?

[stockiNail]

@benmccann if you use .startOf('week').plus({days: weekday - 1}) by Luxon, you have got the same Moment's result: Aug 2.

If I understood correctly, startOf ("2020-08-01", "isoWeek", 7): the result should be 2020-08-02 because I'm requesting the 7th day of the week of that date.

I'm just wondering why the method is called startOf because in my opinion it is not returning any start of. I can change the PR accordingly.

[kurkle]

Its date-fns adapter is doing a different thing then. Its the start of (iso) week that confuses, I'd expect it to be first day of week always. The week starts on different day depending on location, so that's what I'd expect to be configurable.

[stockiNail]

@kurkle you're right. DateFns is doing what I thought, returns the first of the week and you can set the starting point.

Now I don't know anymore which is right and which is not.

If the startOf iso week should return the first day of the week (selected by the time's argument) based on weekday passed as argument, the Luxon (and maybe Moment) should be changed.

startOf ("2020-08-01", "isoWeek", 7) = "2020-07-26"

Instead, If the startOf iso week should return the day of the week (selected by the time's argument) based on weekday passed as argument, the Datefns should be changed.

startOf ("2020-08-01", "isoWeek", 7) = "2020-08-02"

Maybe I see another point of attention. For Datefns, the weekday must be between 0 (Sunday) and 6. For Luxon and Moment the weekday must be between 1 and 7 (Sunday). I don't see any normalization in the adapters.

[benmccann]

Hmm. The docs say that the property is a boolean and not a number, so now we have three ways its being done :smile:

So I guess the question should be how do we want this to work and then we should update everything to match. Should we target the date-fns adapter's behavior and update the rest to match it?

[stockiNail]

@benmccann you have raised an interesting topic that I haven't checked yet.

The doc says

If true and the unit is set to 'week', then the first day of the week will be Monday. Otherwise, it will be Sunday.

that means startOf should return a Sunday or a Monday based on boolean value of isoWeekday property. This sounds to be the use case of startOf therefore to stay simple, without changing too much, could be:

change the startOf adapters in order to return Monday of the week of the passed timestamp if weekday (its modulo by 7) is 1 otherwise Sunday for all the rest of the cases.

Make sense?

[kurkle]

I might be missing something. But ISO week starts from monday by definition and does not have partial weeks. I'm not sure if there is a week numbering system starting from sunday and having no partial weeks. And if there is, are the rules for first week the same (eg. 4th of jan is always on week 1)

US numbering has partial weeks. so Jan 1 is always first day of week, right? But that is the 'week' and not 'isoWeek'.

There are couple of other system in the US too, at least for broadcasting and accounting. The accounting one might be the one that isoWeek starting from sunday is used, but I'm not sure if that works in any/all of the date libs. (Because 1st of Jan is on the first week always in that one)

Middle Eastern system looks like it could be handled by 'week' numbering, but starting from saturday. So should probably have the parameter for 'week' unit too.

[stockiNail]

@kurkle you're right, and it's always an annoying topic.

Nevertheless, due to what the doc is saying clearly, you can start providing Monday or Sunday and maybe the first implementation (we are fixing the missing function on Luxon) could fit the current use case.

And then, you can start thinking which use cases could be addressed to improve it.

[stockiNail]

@kurkle @benmccann I had a look to CHART.JS code where and how isoWeek and startOf are used together.

There are 2 calls into https://github.com/chartjs/Chart.js/blob/master/src/scales/scale.time.js

const {parser, round, isoWeekday} = options; // <-- here is reading the options and isoWeekDay is BOOLEAN
...
if (round) {
    value = round === 'week' && isoWeekday
        ? scale._adapter.startOf(value, 'isoWeek', isoWeekday) // <-- here isoWeekDay is BOOLEAN, not NUMBER
        : scale._adapter.startOf(value, round);
}

and

const weekday = minor === 'week' ? timeOpts.isoWeekday : false; // <-- here is assigned as BOOELAN
....  
// For 'week' unit, handle the first day of week option
if (weekday) {
    first = +adapter.startOf(first, 'isoWeek', weekday); // <-- passed as BOOELAN
}

In both cases, the weekday argument of startOf is always a boolean (or 0-1). I would suggest to change all adapters accordingly with this use case.

Furthermore I'd suggest to change the date adapter doc (in Chart.js) in the code changing the type of argument of startOf. Last but not least, to change the new section for "definition types" applying the new argument type (boolean).

If you agree, I can try (taking time, sorry) to submit the issue and then the PR on all involved projects, starting from this one (luxon-adapter).

Let me know.

[stockiNail]

@benmccann @kurkle updated the PR accordingly with the previous post.

I have also added test cases for startOf isoWeek.

[benmccann]

Yeah, you're right that it's treating isoWeekday as a boolean because if you passed 0 it would not do what you expected. What do you think @kurkle. Would you be okay with changing the adapters to a boolean?

[stockiNail]

@benmccann Just to complete the analysis.

The moment adapter is working if you use isoWeekday: true (looking for Monday) but doesn't work well looking for Sundays.

The date-fns adapter is working well because the weekday start from 0 (Sunday and it's also the numeric representation of false) and 1 is Monday( it's also the numeric representation of true). Let's say luckly :)

[stockiNail]

Would you be okay with changing the adapters to a boolean?

FYI I have already changed and tested the other adapters on my forks, as well, therefore I can push whenever you want.

[kurkle]

I'd prefer the 0-6 or 1-7, because I have an wip app supporting saturday as the first day of week (and using chart.js with date-fns)

[benmccann]

Ok, that sounds fine to me. It is more flexible.

Any preference on 0-6 or 1-7? I don't really have one. The date-fns one already uses 0-6, so that'd be fewer adapters to update :slightly_smiling_face: And it's also more standard programmer's indexing, so I guess I have some slight preference for it

[stockiNail]

Already changed using the week day between 0 (Sunday) and 6 (Saturday). Added also test with true/false (as it is used today by Chart.js) and it works.

[stockiNail]

@kurkle @benmccann I have created 2 issues into moment and date-fns adapter project and the related PRs with the same logic.

[kurkle]

I'm wondering if the trunc/min/max is a bit excessive? It would actually hide an configuration or programming error. A slight performance hit maybe too.

[stockiNail]

I coded like that to avoid mistakes on date calculation. The other option is to throw a RangeError (even if the trunc sounds to be needed as well). In terms of performance, I have some doubts that those statements can decrease the performances too much. Nevertheless I can implement as you want.