Implement .d.ts generation from jsdoc comments

[jannikac]

This is a draft. This PR exists to discuss the automatic generation of a .d.ts file for typescript consumers of this library. The .d.ts file should be generated from the existing jsdoc comments. Some changes have to be made to the jsdoc comments so that the generated .d.ts file has the correct typings (#367 (comment)).

The following snippet was used to test the correctness of the types:

const data = "...some ical data";
const jCalData: any = ICAL.parse(data);
const comp = new ICAL.Component(jCalData, null);
const events = comp.getAllSubcomponents("vevent");
for (const event of events) {
  // event is of type ICAL.Component but it has no methods attached
  event.addSubcomponent();
}

I've just tried to fix the lib/ical/component.js file so that the typing work correctly. I will add the others if this is confirmed to be the way to go. I got it to work by removing the @alias ICAL.Component comment and removing the ICAL. specifier for the Component class. This can be observed in commit #822b5c0.

I've also generated the documentation with npm run jsdoc and ran the tests which worked. The snippet above seems to work with this approach aswell (event has the correct methods attached). So far this looks good to me however there could be other reasons why ICAL.Component is used instead of just Component. Perhaps @kewisch could supply some info on that or if the removal of ICAL. is problematic otherwise. If there are no objections I'll remove ICAL. from the other classes aswell and add the d.ts generation into the build pipeline.

[jannikac]

Just added generation of d.ts files into the rollup config. npm run build should now create the declarations under dist/types. I also added a types field into package.json so npm knows where to find the type declarations. Generation works fine, however the issue with ICAL. stated above still stands.

[coveralls]

Pull Request Test Coverage Report for Build 9501516245

Details

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

---

Totals
Change from base Build 9501503578:	0.007%
Covered Lines:	9408
Relevant Lines:	9569

---

💛 - Coveralls

[kewisch]

Removing ICAL in places where it is used to reference other components might work out, feel free to continue experimenting. What I'm worried about is the generated API docs. If those simply say Component instead of ICAL.Component, consumers of the library might believe they can drop the prefix or adopt patterns like let { Component} = ICAL; and work around the namespacing.

Can you take a look at what the generated api docs look like for the changes you've made and maybe post a screenshot? A compromise might be to use e.g. {Component} in references, but ICAL.Component when writing about it in free text.

You could also experiment with using @typedef to create some aliases, this way we could potentially use ICALComponent in the references, and then ICAL.Component in free text.

I'll try to take a closer look, but maybe we could get someone with more typescript experience to comment since I haven't used it much if at all.

[jannikac]

Sure, this is what the Documentation looks like with @alias and ICAL. removed (links from other functions that return a Component or use a Component work correctly aswell):

If you have the time you can also clone this fork and build the docs locally to see how it looks for yourself to get the full picture.

Other than that it seems that the type hints that jsdoc should give you don't even work with the current implementation as in the ICAL.Component types don't resolve to any type

It seems to me that ICAL.Component works in the docs (meaning it creates a working link) but not in the actual library code itself.

I don't really understand the part with the namespacing you mentioned. Yes a user can do const { Component } = ICAL; and construct a new component with const temp = new Component(""); with my implementation but I don't really see an issue with that, perhaps you can elaborate?

I also experimented a bit with @typedef {Component} ICALComponent and found that it just creates an alias to the Component which is separate from the component definition in the docs. This, in my opinion, is more confusing for the end user because they have to understand that there is the Component class and the ICALComponent class which are essentially the same and just named different for aesthetics right?

I'd be happy to implement the compromise you mentioned for the rest of the codebase: changing all references to Component and using ICAL.Component in free text. I'd also update the remaining classes in a similar manner.

[kewisch]

I don't really understand the part with the namespacing you mentioned. Yes a user can do const { Component } = ICAL; and construct a new component with const temp = new Component(""); with my implementation but I don't really see an issue with that, perhaps you can elaborate?

It is more of an optics thing. Technically of course it works, but this isn't how I intended it to be and I don't want that pattern to establish.

I read a little bit about types and saw there is something like export as namespace ICAL which would define the namespace ICAL globally. I'd be ok with a d.ts file for module.js if that would allow defining the namespace globally in ts (not sure thats how it works?). Can you take a look at that?

[jannikac]

Just to clarify, this the current changes of my fork look like from a library consumer perspective:

Usage in typescript (js would be identical in this example I believe):

// import still works like it used to
import ICAL from "ical.js-test";
const { Component } = ICAL;

const data = "...some ical data";
const jCalData = ICAL.parse(data);
// the type of this comp is still ICAL.Component
const comp = new ICAL.Component(jCalData, null);
// even the type of comp2 is ICAL.Component
const comp2 = new Component(jCalData, null);
const events = comp.getAllSubcomponents("vevent");
for (const event of events) {
  // event also is of type ICAL.Component
  event;
}

So for the library consumer this still looks correct right? The type shows up correctly as ICAL.Component although the jsdoc comments in the library return a Component. This is what you wanted to achieve right? Sorry for asking so much but I'm afraid I don't understand your requirements a 100%. Maybe you can summarize what you want the api documentation and the rest of the libary (imports and such) to look like and I can try to work something out and report back.

I don't think the export as namespace ICAL idea would help here since to my understanding it just creates a global variable so it can be used without importing, but you may still import it with import ICAL from "icaljs" form of import.

[kewisch]

Apologies I haven't gotten back, for some reason I had wrongly assumed I was waiting on a reply from you and skipped over this PR when going through the other issues. The example you show is as expected. Here is my thinking:

• The identifiers Component and Property are very generic, therefore I'd prefer that code written with ICAL.js is always of the form let comp = new ICAL.Component(...) instead of just let { Component } = ICAL; or `import { Component } from "ical.js". I obviously can't stop anyone.
• If the documentation simply refers to Component and Property for technical reasons, then people might assume the standard way to use this library is without the ICAL. prefix, and others learning from examples written by others might propagate it.

Therefore, if the generated documentation could always use e.g. ICAL.Component when referring to components as much as it is possible, I would be happy. To give a few examples:

https://kewisch.github.io/ical.js/api/ICAL.Event.html has a Component parameter, which is referred to as ICAL.Component. With your changes it would simply show Component.

https://kewisch.github.io/ical.js/api/ICAL.Component.html has ICAL.Component in the header, the parent parameter is also ICAL.Component. This is good. The fact that it says new Component(jCal, parent) isn't ideal, but I don't think this can be influenced so it is ok to stay as is. I believe with your changes this would show Component in all locations.

Sorry for making this more complicated than it seems. If you want to find me on Matrix to chat, I am @kewisch:mozilla.org.

[kewisch]

@jannikac Following our discussion on Matrix, I've pushed two commits that should make things work as well as they probably can. I see the dilemma that vscode won't complete the code, and don't see a way to fix this with typescript. Instead, the new jsdoc plugin will change the param names to include the module for the generated docs, while allowing to remove the alias and prefix in the params so typescript completion works. the @link properties apparently need to stay with the full name, otherwise they are not linked in the jsdoc. If you could keep it with the prefix in case it isn't actually linked that would be great (or just go ahead and link it).

To get this to work for me I had to make the following additional changes you should look into:

• In package.json I had to specify the types per exports, otherwise tsc was unhappy at me:

diff --git a/package.json b/package.json
index 23aafc7..ddd973c 100644
--- a/package.json
+++ b/package.json
@@ -63,7 +63,10 @@
     "ghpages": "npm run jsdoc && npm run validator"
   },
   "exports": {
-    "import": "./dist/ical.min.js",
+    "import": {
+      "default": "./dist/ical.min.js",
+      "types": "./dist/types/module.d.ts"
+    },
     "require": "./dist/ical.es5.min.cjs"
   },
   "files": [

• I had to add a few more things to tsconfig to make module resolution work and not have it scan all the files.

diff --git a/tsconfig.json b/tsconfig.json
index b30608e..f41ddfc 100644
--- a/tsconfig.json
+++ b/tsconfig.json
@@ -1,5 +1,8 @@
 {
   "compilerOptions": {
+    "target": "es2021",
+    "module": "nodenext",
+    "moduleResolution": "nodenext",
     // Tells TypeScript to read JS files, as
     // normally they are ignored as source files
     "allowJs": true,
@@ -11,5 +14,6 @@
     // "Go to Definition" in VSCode
     "declarationMap": true,
     "declarationDir": "dist/types"
-  }
-}
\ No newline at end of file
+  },
+  "include": ["lib/ical/**/*.js"]
+}

I didn't actually commit this since I only have a shallow understanding of what I did there, I'd appreciate if you could take a look if this is necessary and complete any other missing pieces such as potential types for the require exports.

[jannikac]

Hey just wanted to post a update on the state of types and a comment for my commits made recently:

Current state of type generation

What works

• most types, getters, properties, returns and parameters work and return the correct type to be used further in typescript

What doesnt work

• Everything that uses a @typedef, so everything that requires or returns these types wont have correct typings
  • occurrenceDetails in Event. So the return of Event.getOccurrenceDetails() doesnt return a correct Event.occurrenceDetails
  • designSet in design.js
  • parserState, however that is just used internally so it wont be an issue for library consumers
  • weekDay in Time. So for example Time.endOfWeek() doesnt return a correct Time..weekDay
  • frequencyValues in Recur. So for example Recur.freq doesnt return a correct Recur.frequencyValues
• the jsdoc plugin tools/jsdoc-ical.cjs currently has all avalible classes added manually to the gIcalClasses Set. Ideally they should be collected from the source files to minimize manual changes when classes are added/removed

I'd say 80-90% of types are generated correctly and avalible to library consumers but the typedef issue still stands. ill try to look into it more but would apreciate some input if someone is reading this.

I'm not sure if @kewisch would want to merge an incomplete implementation of types generation because that would probrably cause more issues to be opened like "Recur.freq doesnt return a type that can be resolved".

Comments on the recently made commits

fb1473f

• moved tsconfig to rollup plugin config as discussed to discourage users to try to generate types with tsc

b098f05

• I believe this jsdoc was just incorrect because there is no zone parameter in the register function, however there is a timezone param

f08fd34

• replace @Class and @alias with @memberof as discussed
• removed @Class in parse.js because it was needed before ES2015 classes were a thing
• removed ICAL. from all the classes in jsdoc to make generated d.ts files work
• some imports were added for example in event so the types were avalible in the resulting d.ts files

b525f35

• dropped ICAL prefix for getters, setters and everything that uses @type(ICAL.*). this makes type resolution work for something like the duration property on Event which returns a duration object. previously the returned duration didnt resolve to the correct ICAL.Duration type.
• hacked the jsdoc plugin to prepend ICAL. to everything that uses a the name of a class. this makes complex types like Array<Component> work

[kewisch]

Almost there, just a few minor changes. Thank you for putting so much effort into this, I'm looking forward to the merge!