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

Invalid typescript declarations #723

Closed
dilyanpalauzov opened this issue Jun 29, 2024 · 4 comments
Closed

Invalid typescript declarations #723

dilyanpalauzov opened this issue Jun 29, 2024 · 4 comments

Comments

@dilyanpalauzov
Copy link
Contributor

As mentioned at #712 (comment) 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
Copy link
Contributor

jannikac commented Jul 1, 2024

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

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

kewisch commented Jul 21, 2024

@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.

@dilyanpalauzov
Copy link
Contributor Author

I think this is addressed now, e.g. by e609077.

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

No branches or pull requests

3 participants