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

isoWeekday(String) inconsistent with isoWeekday(Number) on Sunday #2704

Closed
nvdh opened this Issue Oct 28, 2015 · 6 comments

Comments

Projects
None yet
4 participants
@nvdh
Contributor

nvdh commented Oct 28, 2015

isoWeekday(7) gives me Sunday of this iso week (monday-sunday)
isoWeekday("Sunday") gives me the Sunday of previous iso week

screen shot 2015-10-28 at 07 53 49

version: https://cdnjs.cloudflare.com/ajax/libs/moment.js/2.10.6/moment.min.js

@icambron

This comment has been minimized.

Member

icambron commented Nov 26, 2015

That's correct. This Sunday is isoWeekday(0) and "Sunday" is an alias for 0, not 7.

@icambron icambron closed this Nov 26, 2015

@nvdh

This comment has been minimized.

Contributor

nvdh commented Nov 26, 2015

I figured that's how it's implemented and understand what you mean now, but the result was a bit counter-intuitive from my point of view. The documentation says:

Gets or sets the ISO day of the week with 1 being Monday and 7 being Sunday.
moment().isoWeekday(1); // Monday
moment().isoWeekday(7); // Sunday

So in an ISO week, 7 is always Sunday. The documentation of the alias says:

As of 2.1.0, a day name is also supported. This is parsed in the moment's current locale.
moment().day("Sunday");

So with those two pieces of information it might be a little bit confusing when you do:

moment().isoWeekday("Sunday")

I would expect it to give the same result as moment().isoWeekday(7).

Apart from that, if you say "Sunday" is an alias for 0, this might conflict with the documentation here:

If the locale assigns Monday as the first day of the week, moment().weekday(0) will be Monday. If Sunday is the first day of the week, moment().weekday(0) will be Sunday.

Not trying to brag, just want to explain why this was unclear for me. Maybe a note in the documentation stating that "sunday" is an alias for 0 would be useful.

@icambron

This comment has been minimized.

Member

icambron commented Nov 26, 2015

Fair point. It could be that it should be an alias for 7 or that 0 vs 7 should behave differently. I'm not immersed enough in the details to be sure what the right thing to do is, so I'll just leave this open.

@icambron icambron reopened this Nov 26, 2015

@mj1856

This comment has been minimized.

Member

mj1856 commented May 10, 2016

Taking a look at this again. The docs only talk about being able to pass "Sunday" with regard to the day function, not weekday or isoWeekday. They should probably be updated.

Then, I'd agree that for isoWeekday, the "Sunday" alias should be changed to match 7, not 0. That it currently matches 0 is a bug, since that's the whole point of the isoWeekday function.

@mj1856

This comment has been minimized.

Member

mj1856 commented May 10, 2016

A PR would be welcome. First, submit a PR to fix the behavior of the isoWeekday function. Then, submit a separate PR to https://github.com/moment/momentjs.com with the documentation change.

@mj1856 mj1856 added the Up-For-Grabs label May 10, 2016

nvdh added a commit to nvdh/moment that referenced this issue May 11, 2016

@ichernev ichernev closed this in 2f7af1e Jun 14, 2016

ichernev added a commit that referenced this issue Jun 14, 2016

Merge pull request #3177 from nvdh:bugfix-2704
Bug Fix #2704 - isoWeekday(String) inconsistent with isoWeekday(Number)
@mmonirul

This comment has been minimized.

mmonirul commented Oct 1, 2018

I'm trying to generate date via year, week and day number i.e let date = moment().year(2019).isoWeek(1).isoWeekday(1).toDate(); //Mon Dec 31 2018 11:56:45 GMT+0100 (Central European Standard Time)

the problem i'm facing when i want to get year and week from generated date moment(date).year() | moment(date).isoWeek() I got the wrong. NOT SURE HOW TO SOLVE THIS

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment