fullcalendar / temporal-polyfill

A lightweight polyfill for Temporal, successor to the JavaScript Date object
MIT License
369 stars 14 forks source link

PlainDate.dayOfWeek not adhering to the standard #26

Closed wundersolutions-juanjo closed 9 months ago

wundersolutions-juanjo commented 10 months ago

In the current 0.2.0 version of the polyfill, PlainDate.dayOfWeek is not returning the correct value for ISO 8601 calendars

From the current docs on PlainDate,

The dayOfWeek read-only property gives the weekday number that the date falls on. For the ISO 8601 calendar, the weekday number is defined as in the ISO 8601 standard: a value between 1 and 7, inclusive, with Monday being 1, and Sunday 7. For an overview, see ISO 8601 on Wikipedia.

Usage example:

date = Temporal.PlainDate.from('2006-08-24');
['MON', 'TUE', 'WED', 'THU', 'FRI', 'SAT', 'SUN'][date.dayOfWeek - 1]; // => 'THU'

Testing that on a website using the https://cdn.jsdelivr.net/npm/temporal-polyfill@0.2.0/global.min.js polyfill results on dates being one day off

digaus commented 9 months ago

Just had same observation. Day of week is not returning correct value:

image

Would be nice if this could get a hotfix

digaus commented 9 months ago

my current workarround:

const dayIntl: Intl.DateTimeFormat = new Intl.DateTimeFormat('en-GB', { weekday: 'short' } as Intl.DateTimeFormatOptions);
const day: 'Sun' | 'Mon' | 'Tue' | 'Wed' | 'Thu' | 'Fri'| 'Sat' =  dayIntl.format(temporalDate) as 'Sun' | 'Mon' | 'Tue' | 'Wed' | 'Thu' | 'Fri'| 'Sat';
return [ '', 'Mon', 'Tue', 'Wed', 'Thu', 'Fri', 'Sat', 'Sun' ].indexOf(day);
arshaw commented 9 months ago

I see what's going on here. The code is internally leveraging a zoned Date method (.getDay()) when it shouldn't be. This gives eradic results based on the current user's timezone. I'll get this fixed+released today.

arshaw commented 9 months ago

This is fixed in v0.2.1

Turns out I was incorrectly leveraging the timezone-sensitive Date::getDay() AND I had an off-by-one error when computing days-of-week, and the two bugs cancelled each other out for my particular timezone and the time of day I normally work, so I never experienced the bug (nor my CI). Fun.

arshaw commented 9 months ago

@digaus and @wundersolutions-juanjo could you kindly check to see if things work in your timezone (even tho timezone should not matter)

digaus commented 9 months ago

Yeah timezone should not matter but I can confirm, that it works now correctly with dayOfWeek

wundersolutions-juanjo commented 9 months ago

Looks good, both for #26 and #27, thank you very much!