Generate official TypeScript type definitions

[mramato]

It's been a long requested feature for CesiumJS to have official TypeScript type definitions.

While the community has done a yeoman's job of manually supporting various efforts, the most recent incarnation of which is @types/cesium, the sheer scale and ever-evolving nature of Cesium's code base makes manual maintenance a Sisyphean task.

Thankfully, our decision to maintain meticulous JSDoc API documentation continues to pay dividends and is what makes automatically generating TypeScript definitions possible. Using the excellent https://github.com/englercj/tsd-jsdoc project (and a bunch of cleanup on our end) we can now automatically generate and even partially validate official definitions as part of the build process. (Thanks to @bampakoa who contributed some early PRs to both CesiumJS and tsd-jsdoc over a year ago and is how I learned about tsd-jsdoc)

While tsd-jsdoc output is mostly where we need it to be, we do post-processing on it as well. This lets us clean up the output and also make sure these definitions work whether users include cesium via module, i.e. import { Cartesian3 } from 'cesium', or individual files, i.e. 'import Cartesian3 from 'cesium/Source/Core/Cartesian3'. There were also some quirks of tsd-jsdoc output we fixed that may eventually turn into a PR into that project from us. The post-processing is part typescript compiler API, part string manipulation. It works and is straightforward but we might want to go full TS api in the future if we decide we need to do more complicated tasks. The output of tsd-jsdoc is currently a little noisy because of some incorrect error reporting, but I'm talking with the maintainer in englercj/tsd-jsdoc#133 to get them fixed. No need to hold up this PR for it though.

The definition is generated as a single Cesium.d.ts file in Source, so it lives alongside Cesium.js. It is ignored by git but generated by a separate build-ts task as part of CI and makeZipFile. This task also validates the file by compiling it with TypeScript, so if a developer does anything too egregious, the build will fail. Definitions are automatically included in our npm packages and release zips and will be automatically used by IDEs thanks to the types setting in package.json. This means that IDEs such as VS Code will prefer these types over the existing @types/cesium version by default.

I didn't want to slow the build step down, so I made this a separate step, but in the future we could consider running it by default and we could also unignore this file in Git so that PR reviewers can see the impact, if any, our code changes have on the generated definitions. This might be a good idea as an additional sanity check and should only actually change when the public API itself changes. But the issue would be remembering to run it before submitting the code (or we could use git hooks I suppose?) I just don't want to slow down devs so I'm hesitant to do anything like this out of the gate. We can definitely revisit in the future.

A particular exciting thing about this approach is that it exposed a ton of badness in our current JSDoc markup, which is now fixed. Previously, our only concern was "does the doc look right" and we didn't pay attention to whether the meta information generated by JSDoc correctly captured type
information (because up until it didn't matter). We leaned particular hard on @exports which acted as a catch-all but has now been completely removed from the codebase. All this means is that our documentation as a whole has now improved greatly and will continue to be maintained at this
new higher level thanks to incorporating TS definition creation into our pipeline!

One minor caveat here is that obviously we changed our JSDoc usage to both make it correct and also accommodate TypeScript. The main drawback to these fixes is that enums are now generated as globals in the doc, rather than modules. This means they no longer have their own dedicated page and are instead on the globals page, but I changed the code to ensure they are still in the table of contents that we generate. I think this trade-off is perfectly fine, but I wanted to mention it since it does change the doc some. We can certainly look into whether we can generate enums on their own page if we think that makes sense. (I actually like this approach a little better personally).

Last major piece, the actual code. 99% of the changes in this PR only affect the JSDoc. There are two exceptions:

A few of our enums also have private functions tacked onto them. I had to move these functions to be outside the initializer but otherwise they are unchanged. This ensures that a valid TS enum is generated from our code, since you can't have functions globbed onto enums in the TS world. If we were writing TS code by hand, we could use declaration merging with a namespace, but the toolchain we are using doesn't have a way to express that right now. There were two cases where these extra functions weren't private, ComponentDataType and IndexDataType. That means that as far as the TS definitions goes, the helper methods don't exist. I consder this an edge case and we can write up issues to investigate later. I'm actually not even sure if these functions are public on purposes, @lilleyse can you confirm?

We had a few places where we had method signatures with optional parameters that came before required parameters, which is silly. This is invalid TypeScript (and not good API design no matter the language). In 99% of cases this was equalsEpsilon style functions where the lhs/rhs were optional but the epsilon was not. I remember the discussion around this when we first did it because we were paranoid about defaulting to 0, but it's an edge case and it's silly so I just changed the epsilon functions to default to zero now, problem solved.

Here's a high level summary of the JS changes:

• Use proper @enum notation instead of @exports for enums.

• Use proper @namespace instead of @exports for static classes.

• Use proper @function instead of @exports for standalone functions.

• Fix Promise markup to actually include the type in all cases, i.e. Promise => Promise<void> or Promise<Cartesian3[]>.

• Fix bad markup that referenced types that do not exist (or no longer exist) at the spec level, Image => HTMLImageElement, Canvas => HTMLCanvasElement, etc.. TypedArray in particular does not exist and much be expressed as a lsit of all applicable types, Int8Array|Uint8Array|Int16Array|Uint16Array....

• Use dot notation instead of tilde in callbacks, to better support TypeScript, i.e. EasingFunction~Callback becomes EasingFunction.Callback. The only exception is when we had a callback type that was i.e. exportKml~ModelCallback becomes exportKmlModelCallback (a module global type rather than a type on exportKml). This is because it's not possible to have exportKml be both a function and a namespace in this current toolchain. Not a big deal either
  way since these are meta-types used for defining callbacks but I wanted to mention it.

• There were some edge cases where private types that were referenced in the public API but don't exist in the JSDoc. These were trivial to fix by either tweaking the JSDoc to avoid leaking the type or in some cases, just as PixelDataType, simply exposing the private type as public. I also found a few cases where things were accidentally public, I marked these as private (these were extreme edge cases so I'm not concerned about breaking changes). Appearances took an optional RenderState in their options, I just changed the type to Object which we can clean up further later if we need to.

• Lots of other little misc JSDoc issues that became obvious once we started to generate definitions (duplicate parameters for example).

Thanks again to the community for helping generate ideas and discussions around TS definitions over the last few years and a big thanks to @javagl for helping behind the scenes on this specific effort by evaluating a few different approaches and workaround before we settled on this one (I'm
working on a blog with all of the gory details for those interested).

Finally, while I'm thrilled with how this turned out (all ~41000 lines and 1.9MB of it), I can guarantee we will uncover some issues with the type definitions as more people use it. The good news is that improving it is now just a matter of fixing the JSDoc, which will benefit the community as a whole and not just TS users.

Fixes #2730
Fixes #5717

ToDo based on feedback below:

• enum anchor tags not working
• Update CHANGES
• Define a Proxy interface instead of using DefaultProxy everywhere
• CustomDataSource/GeoJsonDataSource are not assignable to DataSource
• TileMapServiceImageryProvider and WebMapServiceImageryProvider are not assignable to ImageryProvider
• exportKml returns a Promise but object has no properties, so (await exportKml(...)).kmz is a compiler error.
• PropertyBag is missing an index signature so you can't actually read property values.
• GregorianDate constructor takes initial values for all its members but this isn't in the docs. It should allow up to 7 numbers and a boolean.
• TimeIntervalCollection#findInterval and #get can return undefined (if they don't find any matches).
• Cartesian2.packArray, DistanceDisplayCondition.pack, NearFarScalar.pack, Quaternion.pack, and Color#toBytes return Number[] but you should only use the unboxed number pretty much ever, apparently.
• Fix Math/CesiumMath doc inconsistency with master.
• (Maybe post merge) Generated jsdoc @typedef types don't have conformant ts-doc.

[cesium-concierge]

Thanks for the pull request @mramato!

• ✔️ Signed CLA found.
• ❔ CHANGES.md was not updated.
  • If this change updates the public API in any way, please add a bullet point to CHANGES.md.
• ❔ Unit tests were not updated.
  • Make sure you've updated tests to reflect your changes, added tests for any new code, and ran the code coverage tool.

Reviewers, don't forget to make sure that:

• Cesium Viewer works.
• Works in 2D/CV.
• Works (or fails gracefully) in IE11.

[mramato]

Goes to show, no matter how much time you spend writing up an epic and detailed PR, you still always forget to update CHANGES.md. Thanks @cesium-concierge 😄

[mramato]

If anyone would like to test this in their own TS project, you can point cesium to http://cesium-dev.s3-website-us-east-1.amazonaws.com/cesium/typescript-definitions/cesium-1.69.0-typescript-definitions34565.tgz in package.json.

[kring]

I scanned through all the changes and built the TS defs and docs. Looks solid to me.

The only minor problem I ran into is links to enums in the generated documentation go to the top of global.html when you navigate from a search, instead of going directly to that type's part of the document. Looks like this is caused because ?classFilter=pixel being appended after the hash, breaking the link.

I'm going to try building TerriaJS against this, since it's a typescript app that uses a fair bit of Cesium. I'll also wait until tomorrow to merge this to give others a chance to take a look at this PR.

[kring]

I changed ts-conf.json to avoid hardcoding ./node_modules/tsd-jsdoc. When using Cesium in a yarn workspace, and probably lots of other environments as well, tsd-jsdoc won't be found there. Instead we let Node resolve the module in its normal way. This is a general rule of thumb: if you find yourself typing node_modules, you're probably typing a bug.

[kring]

Now I've also changed the "test" part of build-ts to use a tsconfig.json instead of exclusively command-line arguments. This makes it easier to specify "types": [] which stops TypeScript from pulling in @types/*. That's necessary in some environments (like terriajs) because poorly designed dependencies can have conflicting global types (e.g. if you have both @types/jasmine and @types/mocha, for example), causing compile errors that have nothing to do with Cesium. Note that types for explicitly-imported modules will still be included (if Cesium had any). See https://www.typescriptlang.org/tsconfig#types

[kring]

Well, I tried to use this in TerriaJS, and it was kind of a disaster. Some of the problems were in TerriaJS and I fixed them there. Some were in Cesium and I fixed them above. But the big remaining problem that I don't know how to fix is this:
VS Code doesn't seem to know how to find the individual Cesium modules unless you have import cesium from "cesium"; somewhere. Apparently the types in package.json only applies when you import the default module. 😠

To be fair, @types/cesium didn't work in this scenario either. TerriaJS had a generated .d.ts file to make types available for individual modules. So it doesn't need to hold up this PR, but it'd still be really nice to be able to use Cesium in this way without a bunch of nonsense, if we can figure out how to make it work. We could fix it by putting a generated .d.ts file next to every Cesium source file, but hopefully there's a better way.

[kring]

Strangely, I think it's just a VSCode problem. The TerriaJS webpack build seems to be able to find the types. 🤷

[bampakoa]

@kring have you tried using the typeRoots property of tsconfig.json? It had worked for me in some cases where I was using libraries in a Typescript project that were not ES6 compiled.

[thw0rted]

I'm building my (sadly private) project against the AWS tarball now. It's been Typescript from the beginning.

Maybe 2 years ago, I started with some semi-automated typings from @Zuzon and "Harry Nicholls" (no GH listed), and I've been manually updating them based on what's in the changelog at each release. (It only takes a couple minutes every other month or so!) I put them up in a gist a while back but I've been neglecting to keep them up to date. Of course, I'm sure it's diverged from the actual implementation because I focus on the parts of the API that I personally use.

After the new build finished, I got more errors than fit in my scrollback buffer. I'm assuming that at least in some cases my typings were wrong, but I may have some feedback to add here as well. Stay tuned!

[thw0rted]

OK, first Big Thing. Property and its subclasses are... difficult. It really should be generic based on what kind of value it wraps. It also still suffers from a fundamental disagreement with TS on microsoft/TypeScript#2521 (and there's no sign of movement from the TS team).

Basically, you have to choose between two bad options. The first is show: Property | boolean, which allows the unwrapped value to be assigned, so that ent.polygon.show = false works, but ent.polygon.show.getValue(time) is an error because show types as boolean | Property. The second is show: Property, which breaks the former but lets the latter compile. There are no workarounds, other than altering your consuming code to always manually create a Property instance before assignment.

In my typings, in a few commonly used cases, I changed show: Property to show: Property | boolean. That (anti)pattern accounts for a lot of my errors. I'll make some more comments as I find others.

[thw0rted]

Cesium used to expose a top level function called buildModuleUrl. It's never been officially deprecated according to the changelog but it's not in the docs today. Can that be public again? There also used to be a Proxy -- is DefaultProxy the same thing or should there be a more generic Proxy interface?

CustomDataSource and GeoJsonDataSource are not assignable to DataSource; TileMapServiceImageryProvider and WebMapServiceImageryProvider are not assignable to ImageryProvider.

exportKml returns a Promise<object> but object has no properties, so (await exportKml(...)).kmz is a compiler error. In my typings I use overloading like

    function exportKml(options: exportKml.CommonOptions & {kmz: true}): Promise<{kmz:Blob}>;
    function exportKml(options: exportKml.CommonOptions & {kmz: false}): Promise<exportKml.FileDict>;

but I don't know if that's possible from JSDoc. Otherwise, it could at least return a Promise for Blob | {kml: string, externalFiles: {[fileName: string]: Blob}.

My typings export public interfaces for all options-arguments, e.g. the object you pass to the Entity constructor. I find this very helpful because I often have to set a bunch of parameters conditionally so I just pass around a typed structure. Basically, anything that takes an object with more than 2-3 properties should have an exported, named interface. I went with Entity.ConstructorOptions etc but if namespacing is hard, I don't see anything wrong with EntityConstructorOptions.

PropertyBag is missing an index signature so you can't actually read property values.

Most of the properties of Entity should be optional -- all the FooGraphics ones, description, position, etc etc.

GregorianDate constructor takes initial values for all its members but this isn't in the docs. It should allow up to 7 numbers and a boolean.

TimeIntervalCollection#findInterval and #get can return undefined (if they don't find any matches).

Cartesian2.packArray, DistanceDisplayCondition.pack, NearFarScalar.pack, Quaternion.pack, and Color#toBytes return Number[] but you should only use the unboxed number pretty much ever, apparently.

---

OK, that's my first pass. As I said it's a huge error dump so I may well have missed another couple of (types of) problems. Some of these I might be able to just fix for you, and others are going to require discussion. When I have some more cycles later on I'll take a second look.

[thw0rted]

Sorry to keep spamming the thread but I had an idea: if you want to wire up a post-build step that runs some simple TS tests, I could contribute a bunch of test cases that capture the problems I described above. You wouldn't even need a proper test harness, just to run tsc with a config that incorporates the generated typings file and builds the all the .test.ts files (and fails the build if it can't). I haven't looked at the build-process changes you made yet. If it's simple for you to set up something like this, great, I can add test cases for you, and if it's not, I will see if I have cycles later to figure it out myself.

[kring]

@kring have you tried using the typeRoots property of tsconfig.json? It had worked for me in some cases where I was using libraries in a Typescript project that were not ES6 compiled.

@bampakoa I couldn't get typeRoots to work for me, but adding:

"types": ["cesium"]

To my tsconfig.json did the trick, forcing TypeScript to pull in Cesium's types even though it didn't know it needed to. Thanks for the suggestion!

This isn't quite ideal in that it requires an extra step from Cesium users, but it's an excellent workaround.

[kring]

Thanks for all the info @thw0rted!

There are no workarounds, other than altering your consuming code to always manually create a Property instance before assignment.

Yeah I think we've come to the conclusion that we need to force users to do this, due to the typescript limitation you described. This is exactly what Cesium does under-the-hood anyway, so it's no less efficient at runtime, just less efficient to type.

[mramato]

Thanks for the tweaks, @kring. Thanks @thw0rted! Not spam at all, happy to have you dive and and leave valuable feedback.

I'll make sure I mention the "types": ["cesium"] issue for using individual modules both in CHANGES and in the blog.

The only minor problem I ran into is links to enums in the generated documentation go to the top of global.html

This may be something we can fix on our end as part of our jsDoc template. I'll look into it.

OK, first Big Thing. Property and its subclasses are... difficult.

Totally agree that this is unfortunate, especially since the entire implicit conversion idea is taken directly from C#, which was created by some of the same people. However to echo Kevin's comment, we want to play by the TS rules here and as much as possible make the definition match whatever we would have done if we were writing TS directly. Definitely interested in looking into Property as a generic approach, but need to figure out how to express that in JSDoc. If you have any ideas/thoughts I'm all ears (probably a follow-up PR)

Cesium used to expose a top level function called buildModuleUrl

buildModuleUrl has been private since before Cesium hit 1.0. If there is a good reason to expose it to the public, I'm not against it, but please write up an issue and use case. It's definitely outside the scope of this PR.

is DefaultProxy the same thing or should there be a more generic Proxy interface?

So Proxy never actually existed. It was a fiction perpetrated in some of the JSDoc but it was never actually anywhere in the code nor did it have it's own doc page. The actual class, DefaultProxy is basically has a getURL function and that's it so I just changed everything to DefaultProxy to make my life easier. People can still pass their own "Proxy" instances as long as they implement getURL. I'm not against defining a Proxy base interface if people think it's needed. @kring thoughts?

My typings export public interfaces for all options-arguments

Long term I can definitely see moving to this approach in CesiumJS (I've actually always preferred it compared to what we do now). But it's probably outside the scope of this PR (which is just documenting what's there). It's important that we don't create fiction on the TS side of things, I want to TS definitions to mirror the code as much as possible. Knowing what I know now about JSDoc and the TS definition process, this could actually be pretty easy to add once we settle on an approach. It can even happen inline as a @typedef via JSDoc. I'll write up a separate issue for discussion/implementation after the initial PR is merged.

Most of the properties of Entity should be optional -- all the FooGraphics ones, description, position, etc etc.

They already are, or am I missing something?

I think I addressed all of the non-clear-cut feedback. I'll create a task list in the PR description with all of the to-dos that have been pointed out and hopefully have this in shape by end of today.

[mramato]

Sorry to keep spamming the thread but I had an idea: if you want to wire up a post-build step that runs some simple TS tests

I'm not against this idea, and we've thought about doing similar things for Node.js. However, I think it's worth questioning how likely regressions are here once something is fixed. Having a test file with random stuff isn't super useful if we're 99.9% sure nothing will ever break. I'm trying to think of a way you would introduce "regressions" into the TS definitions that aren't also just JSDoc errors. @kring thoughts?

[mramato]

So many of the "Not assignable from" issues are because we follow a pattern like this:

ImageryProvider.prototype.requestImage = DeveloperError.throwInstantiationError;

And TS picks up on this and says "hey, ImageryProvider.requestImage isn't actually the same as IonImagerProvider.requestImage, which has the correctly documented signature:

IonImageryProvider.prototype.requestImage = function (x, y, level, request)

Which leads to

Argument of type 'IonImageryProvider' is not assignable to parameter of type 'ImageryProvider'.
  Property 'proxy' is missing in type 'IonImageryProvider' but required in type 'ImageryProvider'.

This is despite the fact that the TypeScript definition file includes the correct prototype for both ImageryProvider and IonImageryProvider:

requestImage(x: number, y: number, level: number, request?: Request): Promise<HTMLImageElement | HTMLCanvasElement> | undefined;

I think TS is being too smart and ignoring the type definition file and looking at the actual JS code in some cases. I'm not sure if this is a side-affect of allowJS or not (doesn't appear to be).

Either way, I think the correct fix is to actually have ImageryProvider have the correct function signature and just call "DeveloperError.throwInstantiationError()" inside the function instead of this short-hand.

Similarly, the defaultBrightness, etc.. parameters on ImageryProvider are not defined on the implementations. I'm just going to add them to each one for now, long term we can investigate using "real" inheritance.

[thw0rted]

Working back up the page:

I'm assuming that using actual inheritance to fix the "not assignable" errors is outside the scope of this PR, but you already cut over to ES6 for everything, and classes are right there..... 😁

You're probably right about adding a TS testing step, though some find it handy to have a step in your build process that consumes generated typings just to make sure you notice when the typings go horribly wrong for whatever reason. This is a good example of a step that might only make sense to run in CI?

Re: optional properties on Entity, they're optional in the constructor object but they're not optional on the class itself. ent.wall might return undefined, and ent.wall = undefined is (as far as I can tell?) a totally reasonable way to remove the WallGraphics from an entity, but the current defs show wall: WallGraphics;, L19892 in the AWS tarball. This should be either wall?: WallGraphics or wall: WallGraphics | undefined. (There's a difference between these for interfaces but for classes that you new to get an instance, I don't think it actually matters.)

Re: named interfaces for options arguments, I don't look at it as "creating a fiction". I used to default to not exporting named interfaces when I made them for my own convenience, but over time I came to realize that if I used them internally, somebody calling my code could probably use them too. One of the great strengths of TS is IDE integration, and const opts: Entity.ConstructorOptions = {...} does a great job of suggesting properties and catching errors in a way that const opts: any = {...} doesn't. As you say, I think this will be very easy to implement by moving the argument definition into a @typedef.

I use buildModuleUrl to reference the bundled Natural Earth tileset. I'm making a CesiumWidget that runs in an offline environment so the default behavior is no good to me, and otherwise I have to compute the URL myself.

Re: generic Properties, I haven't set up generics with JSDoc but it looks like tsd-jsdoc supports the @template flag from Closure. So then, maybe ent.show could return a Property<boolean>, whose getValue(...) returned boolean -- right now I have to hard-code the return types of individual properties when I know them. If you were writing the typings in TS directly, I'd say make it Property<T=any> so that it continues to work as-is, but I don't know if the Closure flag supports this.

This is actually something I see with Events as well. I made Events generic in my typings, which is handy because then the type of the arguments in a callback get inferred correctly:

class Globe {
  ...
  readonly tileLoadProgressEvent: Event<(length: number) => void>;
  ...
}

globe.tileLoadProgressEvent.addEventListener(queueSize => {
  if(queueSize < 10) { ... }  // Typed correctly as `number`
});

This isn't just nice-to-have, it's actually necessary if you build your TS with the noImplicitAny flag (and you should!), because otherwise I have to go look up and manually specify the type of each callback argument.

If I have some time later on, I might pull this branch and try to do my own Cesium build. If I get it working, I could put together an example of how generics might work using the @template flag. Don't let that hold up the rest of this, though.

[mramato]

classes are right there.

Definitely outside the scope of the PR, and historically actual inheritance has performance issues (and so does ES6 vs ES5). So things like this have to take a measured approach (literally, we need to measure performance impact). I would love to use classes and inheritance throughout Cesium, and eventually we will.

that consumes generated typings just to make sure you notice when the typings go horribly wrong for whatever reason

This PR has that, we run the declaration file through the TS compiler with --noEmit to confirm it's valid TypeScript and that the API is internally consistent. The stuff we can't check (as you already found) is how it interacts in "real world' cases.

This should be either wall?: WallGraphics or wall: WallGraphics | undefined. (There's a difference between these for interfaces but for classes that you new to get an instance, I don't think it actually matters.)

Interesting, thanks. We'll have to dig more into this. @kring any ideas offhand?

If I have some time later on, I might pull this branch and try to do my own Cesium build.

We would certainly be interested to hear what you find. This PR is just step 1 and we will continue to improve things as we go. But we want to get it into an official Cesium release ASAP but will absolutely make significant strides over the next few releases.

[mramato]

Another issue I just uncovered that has me scratching my head.

ImageryProvider.js references the Request object in JSDoc, but does not import the module because it only references it in the doc. None of the imagery providers import the module either and TS is fine, except GoogleEarthEnterpriseImageryProvider which does import Request. Basically this leaves TS screaming about how Request and Request are not the same object (even though from the declaration file they absolutely are).

Turns out VS Code was lying to me.

[mramato]

Realized that Math/CesiumMath docs is inconsistent with master (I really had the face that Math exists as a type in Cesium). Added a task item to fix it before we merge.

[mramato]

Okay, I fixed the CesiumMath/Math issue.

I also fixed the enum anchor tag issue with browsing the docs.

I uncovered doc issue with @typedef, basically the types being generated don't have the expected ts-doc, everything is jammed into the type description instead having doc on individual properties.. The actual type definition is fine, I'm not sure if we can fix it for Monday, but I'll dig in and see.

[thw0rted]

I didn't want to lose track of this so I'm putting it here. I opened englercj/tsd-jsdoc#134 but a bit more digging led me to notice that the config file for tsd-jsdoc needs "plugins": [ "./node_modules/tsd-jsdoc/dist/plugin" ] to be able to support the @template tag. I don't think you're planning on including any generics in the 1.70 release but once you do start, you'll need that option.

As you can see in the linked issue, the good news is that there's actually no processing done to @template, so you can use the (not officially supported) TS syntax to specify constraints and defaults on type parameters, like CallbackType extends Function = Function, and it goes straight into the declaration. I could see using that to create something like Property<WrappedType = any>, which shouldn't be a breaking change.

[mramato]

Thanks for the additional info @thw0rted I'll definitely update out config so stuff "just works" in the future. The main problem I'm trying to fix right now is that the ConstructorOptions and other types we create don't have ideal documentation. The .d.ts file ends up looking like:

/**
 * Initialization options for the BillboardGraphics constructor
 * @property [show = true] - A boolean Property specifying the visibility of the billboard.
 */
type ConstructorOptions = {
    show?: Property | boolean;
};

So show has no intellisense. The correct markup would be

/**
 * Initialization options for the BillboardGraphics constructor
 */
type ConstructorOptions = {
    /**
     * A boolean property specifying the visibility of the billboard.
     * @defaultValue true
     */
    show?: Property | boolean;
};

I'm not sure if this is something on our end or something on the tsd-jsdoc side.

The good news is that this is purely a doc issue and that the code itself is correct, so this may have to wait until 1.71 if there's no obvious problem (I'll write up an issue after trying to play with it some more)

[thw0rted]

In my typings, I use interface ConstructorOptions { ... } instead of type ConstructorOptions = { ... }. I hadn't checked until just now, but it is possible to make that happen with tsd-jsdoc. The interface test case actually declares a bunch of do-nothing runtime code to generate a much smaller typings result, but that does wind up with per-property annotations that I assume would work well with intellisense. The jsdoc page about the tag gives a "virtual comments" example that is more in line with what I had in mind. I don't have time to try it at the moment but maybe it's a good way forward?

[mramato]

Thanks @thw0rted. Me and @kring discussed making these an interface instead and had ultimately decided on a type. Making them an interface in light of this is still possible, but would require quite a change to all of the new ConstrutorOptions so it would be very labor intensive.

I actually have a modified tsd-jsdoc spitting out the right code for typedef, which produces the "ideal" version that I pasted above, so I'm tempted to fork tsd-jsdoc and open a PR (and we could either use the forked version until the PR is merged, or just ship with what we have and then used the fixed version for 1.71 once the PR is accepted.

@kring your thoughts here?

[mramato]

Actually, I think interface may be the right answer because I don't think it's possible to have a @defaultValue on a type (though it appears to work in VS Code).

I think either way, the answer is to merge this PR (assuming no other changes require) and sort it out in the next release.

The type/interface difference should be negligible from a compatibility standpoint with the next release.

[mramato]

@thw0rted I ended up opening that tsd-jsdoc PR to improve property output I mentioned: englercj/tsd-jsdoc#135

[mramato]

FYI, I tweaked CHANGES.md in master.

For anyone who stumbled onto this PR from elsewhere, there's a blog post with a full overview of how we selected this approach and some details about how it works: https://cesium.com/blog/2020/06/01/cesiumjs-tsd/ (Link won't be live until 6/1)

[thw0rted]

Dang it, I knew I was going to miss one of these :-/

[thw0rted]

Hey @mramato , I think during your review you mentioned something about Entity#position not allowing a CallbackProperty (that returns a Cartesian3) but now I can't find your comment. I found a couple of examples of people on StackOverflow doing the same thing. I think I looked into this previously and the only incompatibility between CallbackProperty and PositionProperty is that the former is missing a getValueInReferenceFrame, but actually nobody ever calls that if the defined reference frame for the property is FIXED, so it just kind of sorts itself out. It's certainly possible I misunderstood, though.

[mramato]

I believe there is also a referenceFrame property that is used. It might work right now "by accident" I just wanted to err on the side of caution. We can definitely revisit sooner rather than later.