sebbo2002 / ical-generator

ical-generator is a small piece of code which generates ical calendar files
MIT License
738 stars 162 forks source link

Include VTimezone #122

Closed newtang closed 3 years ago

newtang commented 5 years ago

I would love to see VTIMEZONE included in the ical-generator. Some calendars are fine without them, like iCal, and Google, however Outlook requires a VTIMEZONE section when a timezone is used, otherwise it won't parse. I imagine there are other calendars that are also sticklers as well.

I imagine this could work by bundling in the information from here, possibly as a build step, then include it in the ics file when relevant.

apnerve commented 5 years ago

This would be so much useful!

apnerve commented 5 years ago

BTW, since moment-timezone is already used, the information can be fetched from moment-timezone itself as mentioned in this answer on StackOverflow

Gr8z commented 4 years ago

Working on this now

sebbo2002 commented 3 years ago

Closing this in favor of #198.

sebbo2002 commented 3 years ago

Okay, everything back to the beginning. I first thought we were going to get this built in based on PR #198, but I see this implementation is based on moment-timezone. Which is no longer a dependency.

So we probably need a build step to bundle this information into a file. Or the feature just works if moment-timezone is installed and available. Because actually I don't want to hold timezone tables around in my library.

sebbo2002 commented 3 years ago

Maybe just allow to integrate ical-timezones if required?

adventpit commented 3 years ago

I would be available to look into this. Do I understand correctly, that you would prefer to leave external dependencies out and have the user put in the according VTIMEZONE string from ical-timezones? So an additional method on ICalCalendarto put this entire string in, instead of using the timezone information from the existing ICalCalendar.timezone() and fetching from ical-timezones ourselves?

sebbo2002 commented 3 years ago

@adventpit I would allow the user to give an ical-timezone object in timezone() instead of a string. Just as different date objects are currently allowed. Thus it remains with one method timezone(). Also, this would be required for both ICalCalendar and ICalEvent.

adventpit commented 3 years ago

Okay, so I might be a bit stupid, because I'm not sure if I quite understand how we'd be doing this.

So working on the premise that ical-timezones stays external, that means we do not import ical-timezones, because we want to be as independent as possible, correct?

So firstly sending in an external object from ical-timezones, would require us again to import the package though, because we don't know how the object is built. Theoretically, we could also rebuild it ourselves, but I don't think that is a best practice especially for maintaining. However, there also is no external object on ical-timezones, only a string. Following, this I guess what you're meaning is that we create a new Object, e.g. ICalVtimezone, the user can create and put the VTIMEZONE string from ical-timezones in through ICalCalendar.timezone().

From there on, we'd just include this string, if the input object is ICalVtimezone. Does that approach make sense?

Furthermore, VTIMEZONE is intended to allow multiple components of itself inside of a VCALENDAR, similar to VEVENT. In VEVENT, I don't think we'd require any changes, because the TZID is already present on DTSTART and DTEND. So I think to properly implement VTIMEZONE, it would preferably be an array. I suppose you can workaround by just appending VTIMEZONE string inside our ICalVtimezone object, but that feels hacky imo.

sebbo2002 commented 3 years ago

So working on the premise that ical-timezones stays external, that means we do not import ical-timezones, because we want to be as independent as possible, correct?

Correct.

So firstly sending in an external object from ical-timezones, would require us again to import the package though, because we don't know how the object is built.

All right, I had a few errors in thinking in my cursory answer, sorry. It's best to just forget what I said. I'll have to have another look at the whole thing, hopefully I'll get to it at the weekend. I thought it was a small thing when I quickly looked at it, but yes, you're right, it's not that simple.

sebbo2002 commented 3 years ago

Okay, I have now had time after all, here is my suggestion: 😂

What do you think?.

adventpit commented 3 years ago

Sorry for the delayed answer.

Yes I think this is more accurate, but I still can't wrap my head around it when it comes to the ical-timezones object. When we get tz in cal.timezone({name: 'Europe/Berlin', generator: tz}), how do me know what it can do without importing the library?

sebbo2002 commented 3 years ago

Just save the reference for later usage, or do I miss anything:

in timezone():

this.icalTimezone = tz; // also validate it with a type checker (is* method)

in toString():

if(this.icalTimezone) {
    // use it
}

I'll probably get a chance to put some time in here again on Wednesday, so if you want I can have a look at it on Wednesday too.

adventpit commented 3 years ago

I think it's just me being stupid here.

I was wondering about the type conversion from the object, because we later want to call on it then to do:

tz.getVtimezoneComponent("Europe/Berlin")

So would tz basically be an any object to us or what do we check with the type checker?

I thought of looking into it myself and sending a pull request, once I've understood it completely, but I'm not sure, if I'm ever gonna reach that state xD So sure maybe take a look at it yourself on Wednesday and we can continue from there.

Thanks for your quick replies on the matter, it's a great project you built yourself here!

sebbo2002 commented 3 years ago

Okay, yes, then it's probably better that way. I'll get back to you on Wednesday and maybe bring a solution with me.

sebbo2002 commented 3 years ago

:tada: This issue has been resolved in version 2.0.0-develop.20 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

sebbo2002 commented 3 years ago

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

The release is available on:

Your semantic-release bot :package::rocket: