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

allDay option doesn't reflect timezone #38

Closed
HaNdTriX opened this issue May 1, 2016 · 9 comments
Closed

allDay option doesn't reflect timezone #38

HaNdTriX opened this issue May 1, 2016 · 9 comments
Milestone

Comments

@HaNdTriX
Copy link

HaNdTriX commented May 1, 2016

First of all thanks for this awesome lib.
I noticed that the allday option doesn't work properly.

ical-generator uses the UTC Time to generate the full day.
In a time Zone (e.g. Europe/Berlin) this might result in a different day.

Example:

Sun May 01 2016 00:00:00 GMT+0200 (CEST)
- 2 Hours Offset // UTC Time
-------------------------------------------
= Sat Apr 30 2016 22:00:00 GMT+0200 (CEST)

Code to reproduce:

ical({
    domain: 'sebbo.net',
    prodId: '//superman-industries.com//ical-generator//EN',
    timezone: 'Europe/Berlin',
    events: [{
        start: new Date('Sun May 01 2016 00:00:00 GMT+0200 (CEST)'),
        summary: 'Example Event',
        allDay: true
    }]
});

Solution:

Normally using UTC Dates on the server is of course the right way to go.
But since the value type Date is not able to reflect the timezone properly, I suggest to use local time on this specific use case instead.

Affected Code

@sebbo2002
Copy link
Owner

Thank you for your exemplary bug report, i think I get your point here. Unfortunately, the solution is not a real one:

Case 1 with local time

ical({
    domain: 'sebbo.net',
    prodId: '//superman-industries.com//ical-generator//EN',
    timezone: 'Europe/Berlin',
    events: [{
        start: new Date('Sun May 01 2016 00:00:00 GMT+0200 (CEST)'),
        summary: 'Example Event',
        allDay: true
    }]
})

The start time would be DTSTART;VALUE=DATE:20160501, which would be correct in this case.

Case 2 with local time

ical({
    domain: 'sebbo.net',
    prodId: '//superman-industries.com//ical-generator//EN',
    timezone: 'Brazil Time',
    events: [{
        start: new Date('Sun May 01 2016 00:00:00 UTC'),
        summary: 'Example Event',
        allDay: true
    }]
})

In this case, the start time would be DTSTART;VALUE=DATE:20160430, which is obviously wrong.

To be onest, I have no idea how to handle this properly. But I think using local time is not a solution we should rely on here (UTC-X etc.)

@takkaria
Copy link
Contributor

takkaria commented May 3, 2016

Here's a sketch of one approach to take.

Use moment-timezone, which is a widely-used date library that also understands timezones. It provides an object, 'moment', which wraps the JavaScript native Date object but also provides new functionality, which means it's backwards compatible while still providing extra, better, functionality. For more info the front page of http://momentjs.com/ and the docs are pretty straightforward.

With this library (and the patch below) we can use timezones in three ways:

  1. Use moment-timezone, provide moment-timezone objects with the timezones set.
  2. Use 'timezone' properties (as now), and JavaScript native dates.
  3. Don't use timezones, and just get a 'Z' appended to all dates.

A replacement formatDate function would look like:

var moment = require('moment-timezone');
function formatDate(opener, inputDate, allday, timezone) {
    // Wrap the date in a moment object if it's not already one, so we can use its formatting & timezone functions - ideally this should be done when the start or end date is set on the event object
    var date = moment(inputDate);
    var line = opener;
    if (allday) {
        // All day events can't take timezones, see 3.2.19 of ical spec
        line += ';VALUE=DATE:' + date.format('YYYYMMDD');
    } else {
        // Use the moment timezone, falling back on the timezone property of event
        var timezone = date.tz() || timezone;
        if (timezone) {
            line += ';TZID=' + timezone;
        } else {
            // Set Moment's UTC mode to output with no TZ offset
            date.utc();
        }

        line += ':' + date.format('YYYYMMDDTHHmmss');
        if (!timezone)
            line += 'Z';
    }

    return line;
}

And then the relevant part of the event output function (event.js:652-665]) would be replaced with:

g += formatDate('DTSTART', data.start, data.allDay, data.timezone) + '\r\n';
if (data.end)
    g += formatDate('DTEND', data.end, data.allDay, data.timezone) + '\r\n';
}

if (data.allDay) {
    g += 'X-MICROSOFT-CDO-ALLDAYEVENT:TRUE\r\n';
    g += 'X-MICROSOFT-MSNCALENDAR-ALLDAYEVENT:TRUE\r\n';
}

Testing in the Node REPL with a few different parameters seems to do the right things:

> formatDate('DTSTART', moment.tz('2016-05-01 00:00', 'Europe/Berlin'), true)
'DTSTART;VALUE=DATE:20160501'
> formatDate('DTSTART', moment.tz('2016-05-01 00:00', 'Brazil/West'), true)
'DTSTART;VALUE=DATE:20160501'
> formatDate('DTSTART', new Date('Sun May 01 2016 00:00:00 UTC'), true, 'Brazil/West')
'DTSTART;VALUE=DATE:20160501'
> formatDate('DTSTART', moment.tz('2016-05-01 07:00', 'Brazil/West'))
'DTSTART;TZID=Brazil/West:20160501T070000'
> formatDate('DTSTART', new Date('Sun May 01 2016 00:00:00 UTC'))
'DTSTART:20160501T000000Z'
> formatDate('DTSTART', new Date('Sun May 01 2016 00:00:00 GMT+0200'))
'DTSTART:20160430T220000Z'

@HaNdTriX
Copy link
Author

HaNdTriX commented May 3, 2016

Thanks for your quick and detailed response.
For now I fixed the issue by removing the timezone offset, before passing the date to ical-generator.

if(allDay) {
  startDate.setMinutes(startDate.getMinutes() - startDate.getTimezoneOffset());
}

@wraybowling
Copy link

Using the iCal generator on http://apps.marudot.com/ical/ you're asked to set your timezone before generating the file which lead me to believe that it makes use of that factor for "all day" events.

I think the only factors in play in the file generated below are DTSTART;VALUE=DATE:20160704 to set the start date, perhaps BEGIN:DAYLIGHT, and in relation TZOFFSETFROM:-0500, TZOFFSETTO:-0400, and TZNAME:EDT

BEGIN:VCALENDAR
VERSION:2.0
PRODID:-//www.marudot.com//iCal Event Maker
CALSCALE:GREGORIAN
BEGIN:VTIMEZONE
TZID:America/New_York
TZURL:http://tzurl.org/zoneinfo-outlook/America/New_York
X-LIC-LOCATION:America/New_York
BEGIN:DAYLIGHT
TZOFFSETFROM:-0500
TZOFFSETTO:-0400
TZNAME:EDT
DTSTART:19700308T020000
RRULE:FREQ=YEARLY;BYMONTH=3;BYDAY=2SU
END:DAYLIGHT
BEGIN:STANDARD
TZOFFSETFROM:-0400
TZOFFSETTO:-0500
TZNAME:EST
DTSTART:19701101T020000
RRULE:FREQ=YEARLY;BYMONTH=11;BYDAY=1SU
END:STANDARD
END:VTIMEZONE
BEGIN:VEVENT
DTSTAMP:20160630T205632Z
UID:[email protected]
DTSTART;VALUE=DATE:20160704
SUMMARY:Independence Day
LOCATION:America
END:VEVENT
END:VCALENDAR

@sebbo2002
Copy link
Owner

ical-generator 3.0 now uses moment-timezone, so this issue should be fixed. I also added the above example in the unit tests. You can test the current develop with npm i ical-generator@next.

@billyshena
Copy link

Great news @sebbo2002 ! Is the 3.0 version still available ? Because running npm i ical-generator@next gives me the following error:

capture d ecran 2018-10-17 a 15 02 16

@sebbo2002
Copy link
Owner

@billyshena 3.x is now our main version, so you can run npm i ical-generator to install it…

@billyshena
Copy link

Just got my version updated, thanks @sebbo2002 !

@billyshena
Copy link

billyshena commented Oct 19, 2018

@sebbo2002 We are facing a small issue about timezone parsing.

Here is an example:

The Event is well formatted in the Europe/Paris timezone

const cal = ical({ name: calendarObj.name, timezone: 'Europe/Paris' })
cal.createEvent({ start: ..., end: ..., timezone: 'Europe/Paris' })

The Event is still formatted in Europe/Paris timezone

const cal = ical({ name: calendarObj.name, timezone: 'Europe/Paris' })
cal.createEvent({ start: ..., end: ..., timezone: 'Europe/London' })

I think that the Event timezone should "always take over" the timezone set on the Calendar one.
Is it a bug or just a misunderstanding ?

Thanks,

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

No branches or pull requests

5 participants