Update or remove tsd-jsdoc

[ggetz]

As part of the build process, CesiumJS generates TypeScript definitions based off our jsdoc comments.

While we're generally pretty meticulous about having accurate documentation, there are a few instances of invalid jsdoc that have slipped through. The build-ts scripts, which converts our jsdoc to TypeScript definitions, has recently been spitting out a ton of warnings indicating probable issues with our meta tags.

There is a helpful list of common pitfalls the link above, including:

• Use proper @enum notation for enums.
• Use proper @namespace notation for static classes.
• Use proper @function notation for standalone functions.
• Fix Promise markup to actually include the type in all cases, i.e. Promise => Promise<void> or Promise<Cartesian3[]>.

This list likely covers most of the warnings. We should fix theses to ensure we have accurate documentation and accurate TYpeScript definitions.

[jjhembd]

I captured the warnings in a file to evaluate. There are 1234 warnings, and 1230 of them are "Failed to find parent of doclet...". I think this is a known issue with the tsd-jsdoc plugin.

The plugin repo seems to be dead. I think we might be better off switching to typescript itself to generate the type definitions. Typescript added this functionality shortly after the tsd-jsdoc plugin was developed.

My first quick test of the typescript compiler is throwing a few errors about "requires using private name from module". This may be worth another look when we have time.

[sanjeetsuhag]

While @enum tag is supported by the TypeScript compiler, it doesn't generate a TypeScript enum, instead it generates a namespace in the definition file. Another small issue is the way we export the frozen JS object. For example, see the definitions generated from ArcType.js - (TSPlayground link):

declare const _default: Readonly<{
    /**
     * Straight line that does not conform to the surface of the ellipsoid.
     *
     * @type {Number}
     * @constant
     */
    NONE: number;
    /**
     * Follow geodesic path.
     *
     * @type {Number}
     * @constant
     */
    GEODESIC: number;
    /**
     * Follow rhumb or loxodrome path.
     *
     * @type {Number}
     * @constant
     */
    RHUMB: number;
}>;
export default _default;

If I go from:

export default Object.freeze(ArcType);

to

export default ArcType;

then I lose documentation for the enum values:

export default ArcType;
/**
 * ArcType defines the path that should be taken connecting vertices.
 */
type ArcType = number;
declare namespace ArcType {
    const NONE: number;
    const GEODESIC: number;
    const RHUMB: number;
}

This is with removeComments set to false.

[sanjeetsuhag]

The JSDoc to .d.ts pipeline seems to have several open issues. I left a comment providing an example of the issue we're running into.

[ggetz]

ts-jsdoc is also stopping us from updating our version of jsdoc

[ggetz]

We can use tsc pretty much as a drop in replacement for tsc-jsdoc. It does require some additional cleanup since, unlike tsc-jsdoc, tsc actually evaluates the code and doesn't just rely on the jsdoc comments.

I started work on this in rm-tsd-jsdoc. To finish this out the following needs to be addressed:

• Port the remaining tests from Specs/TypeScript/index.ts to packages/engine/Specs/types/**/*.test-d.ts and packages/widgets/Specs/types/**/&.test-d.ts using tsd for assertions
• Cleanup tsd errors and test failures in packages/engine type definitions
• Cleanup tsd errors and test failures in packages/widgets type definitions
• Import types from the packages into a top-level declarations file for cesium
• Create minimal tsd tests for cesium to verify the imports are working correctly
• Check issues tagged typescript for any issues we may have fixed along the way. For each, we should add a tsd test to prevent regressions just like we would for bugs in the source code.
• Make sure build and release scripts build definitions at the proper time
• Update dev documentation around how we build and test .d.ts files

Some potential follow ups:

• Bundle the root Cesium.d.ts for release: Typescript definitions when Cesium is included via script tag #9254
• Blog post along the lines of Matt's TS post

After that is completed and verified, I think we should also be able to enable type checking via tsconfig.json. But that will also turn up loads of errors so it should probably be a separate effort.

[jjspace]

Some initial issues that are going to be fundamental across the library

Extending classes

Simply put, TS completely ignores the @extends/@augments tag from JSDoc. microsoft/TypeScript#60113 microsoft/TypeScript#26675 microsoft/TypeScript#38985
See a demonstration in this playground

CesiumJS is a very OOP oriented JS library and we make use of extending classes in a decent number of places like Property > CustomProperty and ImageryProvider > BingMapsImageryProvider. Some of these don't "truely" extend the class and others do. AFAIK the "right" way to extend prototypal classes is using Object.create similar to what we do in OpenStreetMapImageryProvider but TS does not at all understand this.

cesium/packages/engine/Source/Scene/OpenStreetMapImageryProvider.js

Lines 120 to 126 in dc667cd

	if (defined(Object.create)) {
	OpenStreetMapImageryProvider.prototype = Object.create(
	UrlTemplateImageryProvider.prototype,
	);
	OpenStreetMapImageryProvider.prototype.constructor =
	OpenStreetMapImageryProvider;
	}

Converting objects into keyword class type classes makes it easy to extends them and then TS properly picks up what the structure should be. See #8359 for more discussion on that but we may have to do this partially for the offending classes.

Speaking with @ggetz she found a way to make it work for the Property family of classes by forcing type's on closures of the parent class like below. This seems to work from the TS side but I have to look into it more to understand that it's doing what we actually want. This may be the solution but needs more investigation. I can say right now though that JSDoc does not like this syntax and fully errors out.

cesium/packages/engine/Source/DataSources/Property.js

Lines 64 to 67 in b4df54b

	Property.prototype.getValue =
	/** @type {(time?: JulianDate, result?: any|any[]) => any|any[]|undefined} */ (
	DeveloperError.throwInstantiationError
	);

Class properties and defineProperties

Most of our classes have one or more properties defined using defineProperties. This is not understood by TS and will result in all of those properties not being defined on the constructed type. (Check Viewer.scene)

The related function defineProperty is understood by TS and will generate the correct types. However, I am unable to find a way for TS to generate types that preserve the documentation that should be attached to these properties. I believe @javagl discovered this as well when initially working on generating types for the library. It's possible we may have to just accept this or it may be a showstopper, open to discussion.

Check this playground example showing the issue and defineProperty working.

I did test around a little with this and was able to get defineProperty working correctly with JSDoc itself by changing how we write the documentation a little like below based on this SO answer

/**
 * Gets the scene.
 * @type {Scene}
 * @name Viewer#scene
 * @readonly
 */
Object.defineProperty(Viewer.prototype, "scene", {
  /** @returns {Scene} */
  get() {
    return this._cesiumWidget.scene;
  },
});

It's not ideal and may still need more adjusting for the JSDoc but it could be the solution we need without fully switching to class classes

Related but similar to @extends TS does not understand or respect @memberOf which we've normally used to defined these properties for JSDoc's purposes microsoft/TypeScript#7237 microsoft/TypeScript#28730

Importing types

There are lots of places we have classes or functions that take a specific type as an argument but don't directly import it in the file itself. This wasn't an issue before because tsd-jsdoc relied only on the JSDoc as the source of truth and could combine all files and make the needed type connections but that's not the case with tsc.

TS understands import('path') statements in JSDoc types like @param {import('./Scene').default} scene but JSDoc does not. An alternative that was generally suggested was to create new @typedefs for the types you want to import. This works but the generated types re-export the type which I do not think is desirable.
It seems there's now a new way to do this in TS 5.5 using @import tags.
So now /** @import { Scene } from "./Scene" */ should work.

This seems good from the TS and types perspective but once again JSDoc does not like this tag because it can't understand it and will fail out. I'm still looking into ways to potentially avoid that because I think this is the solution we ideally want to use.

[javagl]

Is it valid to say that the first points would be completely obsolete if we consistently changed from prototype to class?

I know, doing this consistently may be a considerable effort and a big change. But I'm trying to understand ~"which problem is solved how": We could invest a considerable amount of time to solve the issues of "extending/defineProperties", which also might have to be done incrementally, iteratively, and with careful checks of the resulting state in terms of the .d.ts and the generated HTML docs. Or we could see this as something that could justify the effort and amount of work that has to go into the prototype->class change, iff that change makes the "extending/defineProperties" problem go away as a side-effect.

[ggetz]

Thanks for all the info so far @jjspace!

@javagl I think using class syntax, whether incrementally or holistically, is on the table for consideration in support of this effort. But we should do our due diligence and understand our options before we commit to that path.

@jjspace You mentioned that jsDoc is running into errors with certain tags like import and won't generate.

• What command are you using to build the docs? By default npm run build-docs runs jsdoc with the --pedantic flag, which treats errors as fatal errors, and treats warnings as errors. I would be curious to see what the result is without using that config.
• There are also configuration options for jsdoc that we could possibly adjust.

Regarding importing types, I one of the most important aspects is that we should make sure we are importing types only, rather than the entire module. TypeScript does something called "import elision" that, when generating JS, drops imports which it knows are type-only imports. The effects of this are described in more detail here.

[jjspace]

Re: jsdoc issues

I just pushed a couple commits to the rm-tsd-doc branch that let JSDoc work again with no errors. As I suspected it was very easy to add a custom tag handler for @import and just completely ignore them. I think this will be the way to handle that going forward and the desired way of getting TS to import types. I think this change alone would be good to include in main

I also added a custom transformer to all JSDoc comments that when it finds an arrow funciton => it replaces that type with any. This is not ideal and I left a warning for it but it lets JSDoc not fail out when it encounters this syntax which it does not support. AFAIK the "proper" way to document those types is using @callback but that could result in a bunch of duplicated documentation if we actually do need to do those closures to typecast things so TS is happy for inheritance.

Another thing I noticed at the same time is that we were not generating any documentation for objects with the @interface tag. I quickly added this and set it up to mostly mimic @class pages just so something generates. This was really confusing me when the Property page was not generating because I missed that @ggetz added the @interface tag.
Side note: I think TS might recognize that tag and it might help with the extends issues so this might actually be needed but I have to look into that more.

[jjspace]

Is it valid to say that the first points would be completely obsolete if we consistently changed from prototype to class?

@javagl Given my current understanding that is correct. I modified one of my examples above to include the same class using class instead of prototype and defineProperties and all the types are generated correctly with the corresponding JSDocs.

I'm still going to spend a little more time trying to see if I can get it working without the class refactor but it's possible that this becomes a sort of "forcing factor" for that change.

[ggetz]

I also added a custom transformer to all JSDoc comments that when it finds an arrow funciton => it replaces that type with any. This is not ideal and I left a warning for it but it lets JSDoc not fail out when it encounters this syntax which it does not support. AFAIK the "proper" way to document those types is using @callback but that could result in a bunch of duplicated documentation if we actually do need to do those closures to typecast things so TS is happy for inheritance.

Hm, maybe there's a way we can define the function parameter and return types without using arrow functions?

[jjspace]

Hm, maybe there's a way we can define the function parameter and return types without using arrow functions?

As I mentioned this is achievable with the @callback tag. Check the dropdown for a code example but it may be a moot point given other decisions

JSDoc callbacks

Showing one of the previously added type casts.

Instead of

Property.prototype.getValue =
  /** @type {(time?: JulianDate, result?: any|any[]) => any|any[]|undefined} */ (
    DeveloperError.throwInstantiationError
  );

This works

/**
 * @callback PropertyGetValue
 * @param {JulianDate} [time=JulianDate.now()] The time for which to retrieve the value. If omitted, the current system time is used.
 * @param {any|any[]} [result] The object to store the value into, if omitted, a new instance is created and returned.
 * @returns {any|any[]|undefined} The modified result parameter or a new instance if the result parameter was not supplied.
 */
/**
 * Gets the value of the property at the provided time.
 * @function
 *
 * @param {JulianDate} [time=JulianDate.now()] The time for which to retrieve the value. If omitted, the current system time is used.
 * @param {any|any[]} [result] The object to store the value into, if omitted, a new instance is created and returned.
 * @returns {any|any[]|undefined} The modified result parameter or a new instance if the result parameter was not supplied.
 */
Property.prototype.getValue = /** @type {PropertyGetValue} */ (
  DeveloperError.throwInstantiationError
);

And generates this extra type in the type definitions

export type PropertyGetValue = (time?: JulianDate | undefined, result?: any | any[]) => any | any[] | undefined;

This makes TS and JSDoc happy but I feel like the duplicated docs are bad. There may be a way to combine them but I didn't dig too deep. I also think defining these extra types at the JSDoc level just to appease TS could be a code smell. I did not verify yet if they get generated in the docs as well which could pollute them.

[javagl]

I also think defining these extra types at the JSDoc level just to appease TS could be a code smell.

I might be missing some context, but ... looking at that export, this might not be a "code smell", but rather a "code stench" 😁 : If I understood that correctly, then TypeScript users would see this type being publicly exported, and might therefore rely on this type to exist (meaning that any change here would be "breaking", in the strictest sense...)

[jjspace]

The more time I spend trying to find a way to minimally make both TS and JSDoc happy with our class definitions the more it feels like the logical, and necessary, route forward is to use keyword class classes instead. The closest I've been able to figure out about Object.defineProperties is to move to defineProperty calls instead. If we're already going to be doing a refactor on that scale I think it would be a better use of our time to do the class switch instead.

Some relevant issues I've found around the define___ functions:

• Add typed return for Object.defineProperties microsoft/TypeScript#41424
• Accept generics for defineProperty microsoft/TypeScript#42424
• [NewErrors] 4.3.0-dev.20210412 vs 4.2.4 microsoft/TypeScript#43651 (comment)

  People with // @ts-check are used to seeing+ignoring errors, and if they're heavy checkJS users, they'll have already switched to classes or Object.defineProperty since those are supported.

• Object.defineProperty doesn't define properties on 'this' microsoft/TypeScript#28694
• Allow Object.defineProperty() to definitely assign properties microsoft/TypeScript#42919

I'm going to leave a comment over in #8359 about a path forward to keep the two issues more focused