Fix remaining typescript issues

[jannikac]

Hi,
according to #707 there are some remaining issues with the generated typescript types. This PR aims to fix everything except those regarding ICAL.design.designSet since that is currently broken. Some commits need an explanation:

• Remove @readonly tags because it causes this error in typescript: readonly' modifier can only appear on a property declaration or index signature.. When the getters get converted to typescript, typescript changes @readonly to the typescript keyword readonly which is only allowed in index signatures or properties. However you use the @readonly tag on getter functions which is not valid in typescript. I couldn't find a way to omit specific valid jsdoc comments from being parsed by typescript (@ts-ignore doesnt work and @ignore removed the function from the docs).
• Made the options object optional because otherwise it causes A required parameter cannot follow an optional parameter.. Also I took a look at the code and believe that options itself and their properties should be optional anyways since options.strictExceptions is set in the property declaration to false and options.exceptions states that "If not specified exceptions will automatically be set in relation of compoent's parent"

[coveralls]

Pull Request Test Coverage Report for Build 9546551429

Details

• 4 of 4 (100.0%) changed or added relevant lines in 2 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage remained the same at 98.169%

---

Totals
Change from base Build 9544239438:	0.0%
Covered Lines:	9403
Relevant Lines:	9564

---

💛 - Coveralls

[jannikac]

Managed to fix all the remaining typescript errors, even the designSet ones!

PS: I have updated the tests, so everything passes, although I'm not too familiar with them, please check, if I did that correctly. Also I could open a separate PR for the breaking change if you want, however then one typescript issue will remain unfixed after merging this PR.

[coveralls]

Pull Request Test Coverage Report for Build 9565191609

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

• For more information on this, see Tracking coverage changes with pull request builds.
• To avoid this issue with future PRs, see these Recommended CI Configurations.
• For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

• 100 of 100 (100.0%) changed or added relevant lines in 8 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.007%) to 98.176%

---

Totals
Change from base Build 9544239438:	0.007%
Covered Lines:	9445
Relevant Lines:	9606

---

💛 - Coveralls

[kewisch]

I'm not a big fan of objects for arguments unless it is very many, as in many cases consumers need to create an object solely for the purpose of passing arguments and then it is destroyed again.

I think we should make it as follows:

• First argument is Component|Timezone and required.
• Second argument is String (name) and optional
• If no name is specified, then name is assumed to be timezone.tzid, regardless of if a timezone is passed or a component that is turned into a timezone.
• Runtime error thrown if neither timezone.tzid is set nor name is passed.
• To avoid a breaking change over argument order, a runtime check to see if arg1 is string and arg2 is Timezone, then just swap them correctly. Add a note to the v3 issue to remove the hack when we are ready for v3. Technically it already breaks typescript consumers, but we didn't have types a few weeks ago and the type checks would catch it.

[dilyanpalauzov]

I tried the newest types.

My code so far used new ICAL.Duration({days: -1}) without a problem. But the types say this is now invalid, as it misses isNegative, weeks, hours, … elements. Are hours, weeks required in the object, passed to the constructor?

I guess the parameter of event.iterator() should be optional.

In Time.fromJSDate() the second parameter used to be optional - skipping it, was equivalent to passing “false”. Now this parameter is mandatory. Can it be made optional/default value to be false?

While the types enforce that Component.getFirstPropertyValue() returns string, in my existing code I assume that getFirstPropertyValue('dtend' / 'duration' / 'rrule' ) return Time / Duration / Recur object. Please check, if the return type can indeed be non-string?

Isn’t getAllSubcomponents(name?: string | undefined): Component[]; redundant - name is either optional, or is (a string or undefined)?

[jannikac]

Hi. I just updated the register function with your suggestion. Should I add a test for the new runtime error aswell?

[kewisch]

Fixing the build warning and these two minor comments and we're good, thank you!

[kewisch]

Hi. I just updated the register function with your suggestion. Should I add a test for the new runtime error aswell?

Tests are always well received and maintain coverage, so this would be great!

[jannikac]

Actually I think this test case already covers it, so I didn't add another one:

test('with only invalid component', function() {
      assert.throws(function() {
        let comp = new ICAL.Component('vtoaster');
        subject.register(comp);
      }, "neither a timezone nor a name was passed");
    });

Accepted your changes and fixed the warning with an unused import, so now we should be good

[coveralls]

Pull Request Test Coverage Report for Build 9711732846

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

• For more information on this, see Tracking coverage changes with pull request builds.
• To avoid this issue with future PRs, see these Recommended CI Configurations.
• For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

• 106 of 106 (100.0%) changed or added relevant lines in 8 files are covered.
• 1 unchanged line in 1 file lost coverage.
• Overall coverage decreased (-0.009%) to 98.16%

---

Files with Coverage Reduction	New Missed Lines	%
lib/ical/timezone_service.js	1	98.69%

Totals
Change from base Build 9572782371:	-0.009%
Covered Lines:	9454
Relevant Lines:	9616

---

💛 - Coveralls

[kewisch]

Thank you so much, I've merged the changes and will make a note in the v3 issue.