kewisch / ical.js

Javascript parser for ics (rfc5545) and vcard (rfc6350) data
https://kewisch.github.io/ical.js/
Mozilla Public License 2.0
996 stars 138 forks source link

Invalid typescript declarations #723

Open dilyanpalauzov opened 3 months ago

dilyanpalauzov commented 3 months ago

As mentioned at https://github.com/kewisch/ical.js/pull/712#issuecomment-2179337109 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 commented 3 months ago

Hi,

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?

I believe the jsdoc are indeed incorrect. I created a PR to fix that.

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

This is a bit redundant because the ? means the type string? is defined to be of string | undefined. We currently document the every optional parameter with this syntax @param {=Number} optionalParameter which causes it to be documented in typescript with optionalParameter?: number | undefined. Documenting this with @param {Number} [optionalParameter] creates optionalParameter?: number in typescript which would be clearer. However we'd have to change a lot of code to make this a consistent change, maybe @kewisch can decide?

dilyanpalauzov commented 3 months ago
let zones = Object.create(null)
zones['uu']

returns undefined.

I think TimezoneService.get() should return either a Timezone object, or undefined. The current types mandate that the returned thing is either a Timezone object, or null.

kewisch commented 2 months ago

@jannikac I'm fine switching to use of [optionalParameter]. I used {Number=} randomly because I preferred that syntax at the time, but I don't have strong feelings.