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

Include VTimezone #122

Closed
newtang opened this issue Nov 3, 2018 · 17 comments · Fixed by #243
Closed

Include VTimezone #122

newtang opened this issue Nov 3, 2018 · 17 comments · Fixed by #243

Comments

@newtang
Copy link
Contributor

newtang commented Nov 3, 2018

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.

@sebbo2002 sebbo2002 added the upvote-if-needed I think this is not a feature a lot of people require. Upvote this issue if you disagree… label Jan 7, 2019
@apnerve
Copy link

apnerve commented Jul 12, 2019

This would be so much useful!

@apnerve
Copy link

apnerve commented Jul 13, 2019

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

@sebbo2002 sebbo2002 added pull-request-welcome todo and removed upvote-if-needed I think this is not a feature a lot of people require. Upvote this issue if you disagree… labels Aug 7, 2019
@Gr8z
Copy link

Gr8z commented Jul 16, 2020

Working on this now

@Gr8z Gr8z mentioned this issue Jul 16, 2020
@sebbo2002 sebbo2002 added this to the v2.0.0 🎉 milestone Nov 22, 2020
@sebbo2002
Copy link
Owner

Closing this in favor of #198.

@sebbo2002 sebbo2002 removed the todo label Mar 21, 2021
@sebbo2002
Copy link
Owner

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 sebbo2002 reopened this Mar 23, 2021
@sebbo2002 sebbo2002 removed this from the v2.0.0 🎉 milestone Mar 23, 2021
@sebbo2002
Copy link
Owner

Maybe just allow to integrate ical-timezones if required?

@adventpit
Copy link

adventpit commented Apr 8, 2021

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
Copy link
Owner

@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
Copy link

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
Copy link
Owner

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
Copy link
Owner

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

  • The method timezone() of the ICalCalendar object is extended so that optionally ical-timezone can be passed. Something like cal.timezone({name: 'Europe/Berlin', generator: tz})
  • The event method remains as it is. A string is assumed with the timezone.
  • ICalCalendar's toString() method is extended so that VTIMEZONEs of the used timezones are generated and inserted in the calendar if the reference to ical-timezone was passed accordingly before. Care must be taken that each time zone is inserted only once. If the time zone was not listed, there is no VTIMEZONE object in toString()'s output

What do you think?.

@adventpit
Copy link

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
Copy link
Owner

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
Copy link

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
Copy link
Owner

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 sebbo2002 added this to the v2.0.0 🎉 milestone Apr 14, 2021
@sebbo2002 sebbo2002 linked a pull request Apr 14, 2021 that will close this issue
@github-actions github-actions bot mentioned this issue Apr 14, 2021
sebbo2002 pushed a commit that referenced this issue Apr 14, 2021
# [2.0.0-develop.20](v2.0.0-develop.19...v2.0.0-develop.20) (2021-04-14)

### Bug Fixes

* **Tools:** Prevent formatDate() from using global timezones prefixed with a slash ([85ab7b2](85ab7b2))

### Features

* **Calendar:** Add support for external VTimezone generator ([f4bc8e0](f4bc8e0)), closes [#122](#122)
@sebbo2002
Copy link
Owner

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

The release is available on:

Your semantic-release bot 📦🚀

sebbo2002 pushed a commit that referenced this issue Apr 28, 2021
# [2.0.0](v1.15.4...v2.0.0) (2021-04-28)

### Bug Fixes

* **package.json:** add temporary version ([0bc117e](0bc117e))
* Allow to set null values within object constructors ([8b87183](8b87183))
* **deps:** Also define libs as devDependency for tests ([c04ae32](c04ae32))
* **deps:** Define supported libs as peerDependencies ([84e2784](84e2784))
* Make peer dependencies optional ([b384ac7](b384ac7)), closes [#244](#244)
* **Tools:** Prevent formatDate() from using global timezones prefixed with a slash ([85ab7b2](85ab7b2))
* **deps:** Put necessary typings in peerDependencies as well :/ ([14f0f43](14f0f43))
* **Event:** Remove `moment` dependency in constructor ([8331d4c](8331d4c)), closes [#234](#234)

### Code Refactoring

* **Calendar:** Remove moment.Duration from `ttl()` method ([c6ccd12](c6ccd12))
* Update error URLs ([2aedf55](2aedf55))

### Features

* **Event:** Add `priority()` method ([247039f](247039f)), closes [#163](#163)
* **Attendee:** Add `x()` method for custom attributes ([5d9d686](5d9d686)), closes [#183](#183)
* **Calendar:** add new clear method ([1ebefcb](1ebefcb)), closes [#188](#188)
* Add ReleaseBot ([2fba164](2fba164))
* **Calendar:** Add support for external VTimezone generator ([f4bc8e0](f4bc8e0)), closes [#122](#122)
* **Event:** Allow `X-APPLE-STRUCTURED-LOCATION` without address ([4e63e29](4e63e29)), closes [#236](#236)
* **Event:** Make organizer.email optional ([8450492](8450492)), closes [#137](#137)
* **Event:** Merge `location()`, `appleLocation()` and `geo()` ([62c1516](62c1516)), closes [#187](#187)
* Merge event's `description()` and `htmlDescription()` ([ce537f8](ce537f8))
* Support moment.js, Day.js and Luxon ([#91](#91), BREAKING CHANGE) ([6db24ee](6db24ee))
* **Event:** Support RRule objects and raw strings in `repeating()` ([4436785](4436785)), closes [#190](#190)
* Updated the entire codebase to Typescript ([d013dc0](d013dc0))
* **Events:** Use uuid-random for random UUIDs (close [#215](#215)) ([a4c19cc](a4c19cc))

### BREAKING CHANGES

* Some error messages changed, so if you check for error , please double check them now.
* `htmlDescription()` was removed, use `description()` instead.
* **Calendar:** `ttl()` will now return a number, not a `moment.Duration`. You can still use `moment.Duration` to set the `ttl` value.
* **Event:** `geo()` and `appleLocation()` are not available anymore, use `location()` instead and pass an location object (with title, radius, etc.)
* **Calendar:** Calendar's `clear()` method is a completely new implementation and, unlike previous versions, will not reset metadata such as `name` or `prodId`. Only the events will be removed
@sebbo2002
Copy link
Owner

🎉 This issue has been resolved in version 2.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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