Skip to content

Commit

Permalink
Generate official TypeScript type definitions
Browse files Browse the repository at this point in the history
It's been a long requested feature for us 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 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
  • Loading branch information
mramato committed May 27, 2020
1 parent d49c0c3 commit 85c78ed
Show file tree
Hide file tree
Showing 277 changed files with 1,150 additions and 1,363 deletions.
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ Thumbs.db
/Apps/Sandcastle/templates/bucket.css

/Source/Cesium.js
/Source/Cesium.d.ts

/Specs/SpecList.js
/Source/Shaders/**/*.js
Expand Down
1 change: 1 addition & 0 deletions .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ script:
- npm --silent run prettier-check

- npm --silent run build
- npm --silent run build-ts
- npm --silent run coverage -- --browsers FirefoxHeadless --webgl-stub --failTaskOnError --suppressPassed

- travis_wait 20 npm --silent run makeZipFile -- --concurrency 1
Expand Down
2 changes: 1 addition & 1 deletion Apps/Sandcastle/gallery/Custom Geocoder.html
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@
* The function called to geocode using this geocoder service.
*
* @param {String} input The query to be sent to the geocoder service
* @returns {Promise<GeocoderService~Result[]>}
* @returns {Promise<GeocoderService.Result[]>}
*/
OpenStreetMapNominatimGeocoder.prototype.geocode = function (input) {
var endpoint = "https://nominatim.openstreetmap.org/search";
Expand Down
21 changes: 16 additions & 5 deletions Documentation/Contributors/DocumentationGuide/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ Generally, just follow the patterns that are already in comparable parts of the
- [Property](#property)
- [Property Getter/Setter](#property-gettersetter)
- [Standalone Function](#standalone-function)
- [TypeScript type definitions](#typescript)

## Building the Doc

Expand Down Expand Up @@ -52,7 +53,7 @@ Consider one of the simplest functions in CesiumJS, `defined`:

```javascript
/**
* @exports defined
* @function
*
* @param {*} value The object.
* @returns {Boolean} Returns true if the object is defined, returns false otherwise.
Expand All @@ -70,7 +71,7 @@ function defined(value) {
```

- The doc for `defined` is in the comment starting with `/**`. JSDoc tags begin with `@`.
- `@exports` describes the name of the function that is exported from the module.
- `@function` tells JSDoc that this is a function.
- `@param` describes the function's parameters, and `@returns` describes the function's return value.
- `@example` describes a code sample.

Expand Down Expand Up @@ -379,7 +380,7 @@ Cartesian4.fromArray = Cartesian4.unpack;
/**
* Sort the items in the queue in-place.
*
* @param {Queue~Comparator} compareFunction A function that defines the sort order.
* @param {Queue.Comparator} compareFunction A function that defines the sort order.
*/
Queue.prototype.sort = function (compareFunction) {
if (this._offset > 0) {
Expand All @@ -393,7 +394,7 @@ Queue.prototype.sort = function (compareFunction) {

/**
* A function used to compare two items while sorting a queue.
* @callback Queue~Comparator
* @callback Queue.Comparator
*
* @param {*} a An item in the array.
* @param {*} b An item in the array.
Expand Down Expand Up @@ -535,7 +536,7 @@ DESCRIPTION.
```
DESCRIPTION.

@exports NAME
@function

@param {TYPE} NAME DESCRIPTION.
@param {TYPE|OTHER_TYPE} NAME DESCRIPTION WITH LONG
Expand All @@ -552,3 +553,13 @@ DESCRIPTION.

[@private]
```

# TypeScript

We also use JSDoc to build official TypeScript type definitions. Normally this behavior is transparent to the developer and happens as part of CI, however incorrect or non-standard JSDoc can lead to failures. If CI is failing because of the `build-ts` step, you can debug it locally by running:

```
npm run build-ts
```

In most cases, the TypeScript compiler will provide a very obvious error and line number which will help you track down the offending, most likely incorrect, JSDoc.
2 changes: 1 addition & 1 deletion Source/Core/ApproximateTerrainHeights.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ var ApproximateTerrainHeights = {};

/**
* Initializes the minimum and maximum terrain heights
* @return {Promise}
* @return {Promise<void>}
*/
ApproximateTerrainHeights.initialize = function () {
var initPromise = ApproximateTerrainHeights._initPromise;
Expand Down
2 changes: 1 addition & 1 deletion Source/Core/ArcGISTiledElevationTerrainProvider.js
Original file line number Diff line number Diff line change
Expand Up @@ -459,7 +459,7 @@ ArcGISTiledElevationTerrainProvider.prototype.getTileDataAvailable = function (
* @param {Number} x The X coordinate of the tile for which to request geometry.
* @param {Number} y The Y coordinate of the tile for which to request geometry.
* @param {Number} level The level of the tile for which to request geometry.
* @returns {undefined|Promise} Undefined if nothing need to be loaded or a Promise that resolves when all required tiles are loaded
* @returns {undefined|Promise<void>} Undefined if nothing need to be loaded or a Promise that resolves when all required tiles are loaded
*/
ArcGISTiledElevationTerrainProvider.prototype.loadTileDataAvailability = function (
x,
Expand Down
2 changes: 1 addition & 1 deletion Source/Core/ArcType.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/**
* ArcType defines the path that should be taken connecting vertices.
*
* @exports ArcType
* @enum {Number}
*/
var ArcType = {
/**
Expand Down
2 changes: 1 addition & 1 deletion Source/Core/AttributeCompression.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ var LEFT_SHIFT = 256.0;
/**
* Attribute compression and decompression functions.
*
* @exports AttributeCompression
* @namespace AttributeCompression
*
* @private
*/
Expand Down
2 changes: 1 addition & 1 deletion Source/Core/BingMapsApi.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import defined from "./defined.js";
* or {@link BingMapsGeocoderService}. You can create your own key at
* {@link https://www.bingmapsportal.com/}.
*
* @exports BingMapsApi
* @namespace BingMapsApi
*/
var BingMapsApi = {};

Expand Down
2 changes: 1 addition & 1 deletion Source/Core/BingMapsGeocoderService.js
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ Object.defineProperties(BingMapsGeocoderService.prototype, {
* @function
*
* @param {String} query The query to be sent to the geocoder service
* @returns {Promise<GeocoderService~Result[]>}
* @returns {Promise<GeocoderService.Result[]>}
*/
BingMapsGeocoderService.prototype.geocode = function (query) {
//>>includeStart('debug', pragmas.debug);
Expand Down
4 changes: 2 additions & 2 deletions Source/Core/Cartesian2.js
Original file line number Diff line number Diff line change
Expand Up @@ -665,7 +665,7 @@ Cartesian2.equalsArray = function (cartesian, array, offset) {
*
* @param {Cartesian2} [left] The first Cartesian.
* @param {Cartesian2} [right] The second Cartesian.
* @param {Number} relativeEpsilon The relative epsilon tolerance to use for equality testing.
* @param {Number} [relativeEpsilon=0] The relative epsilon tolerance to use for equality testing.
* @param {Number} [absoluteEpsilon=relativeEpsilon] The absolute epsilon tolerance to use for equality testing.
* @returns {Boolean} <code>true</code> if left and right are within the provided epsilon, <code>false</code> otherwise.
*/
Expand Down Expand Up @@ -745,7 +745,7 @@ Cartesian2.prototype.equals = function (right) {
* <code>false</code> otherwise.
*
* @param {Cartesian2} [right] The right hand side Cartesian.
* @param {Number} relativeEpsilon The relative epsilon tolerance to use for equality testing.
* @param {Number} [relativeEpsilon=0] The relative epsilon tolerance to use for equality testing.
* @param {Number} [absoluteEpsilon=relativeEpsilon] The absolute epsilon tolerance to use for equality testing.
* @returns {Boolean} <code>true</code> if they are within the provided epsilon, <code>false</code> otherwise.
*/
Expand Down
4 changes: 2 additions & 2 deletions Source/Core/Cartesian3.js
Original file line number Diff line number Diff line change
Expand Up @@ -742,7 +742,7 @@ Cartesian3.equalsArray = function (cartesian, array, offset) {
*
* @param {Cartesian3} [left] The first Cartesian.
* @param {Cartesian3} [right] The second Cartesian.
* @param {Number} relativeEpsilon The relative epsilon tolerance to use for equality testing.
* @param {Number} [relativeEpsilon=0] The relative epsilon tolerance to use for equality testing.
* @param {Number} [absoluteEpsilon=relativeEpsilon] The absolute epsilon tolerance to use for equality testing.
* @returns {Boolean} <code>true</code> if left and right are within the provided epsilon, <code>false</code> otherwise.
*/
Expand Down Expand Up @@ -1152,7 +1152,7 @@ Cartesian3.prototype.equals = function (right) {
* <code>false</code> otherwise.
*
* @param {Cartesian3} [right] The right hand side Cartesian.
* @param {Number} relativeEpsilon The relative epsilon tolerance to use for equality testing.
* @param {Number} [relativeEpsilon=0] The relative epsilon tolerance to use for equality testing.
* @param {Number} [absoluteEpsilon=relativeEpsilon] The absolute epsilon tolerance to use for equality testing.
* @returns {Boolean} <code>true</code> if they are within the provided epsilon, <code>false</code> otherwise.
*/
Expand Down
4 changes: 2 additions & 2 deletions Source/Core/Cartesian4.js
Original file line number Diff line number Diff line change
Expand Up @@ -735,7 +735,7 @@ Cartesian4.equalsArray = function (cartesian, array, offset) {
*
* @param {Cartesian4} [left] The first Cartesian.
* @param {Cartesian4} [right] The second Cartesian.
* @param {Number} relativeEpsilon The relative epsilon tolerance to use for equality testing.
* @param {Number} [relativeEpsilon=0] The relative epsilon tolerance to use for equality testing.
* @param {Number} [absoluteEpsilon=relativeEpsilon] The absolute epsilon tolerance to use for equality testing.
* @returns {Boolean} <code>true</code> if left and right are within the provided epsilon, <code>false</code> otherwise.
*/
Expand Down Expand Up @@ -843,7 +843,7 @@ Cartesian4.prototype.equals = function (right) {
* <code>false</code> otherwise.
*
* @param {Cartesian4} [right] The right hand side Cartesian.
* @param {Number} relativeEpsilon The relative epsilon tolerance to use for equality testing.
* @param {Number} [relativeEpsilon=0] The relative epsilon tolerance to use for equality testing.
* @param {Number} [absoluteEpsilon=relativeEpsilon] The absolute epsilon tolerance to use for equality testing.
* @returns {Boolean} <code>true</code> if they are within the provided epsilon, <code>false</code> otherwise.
*/
Expand Down
8 changes: 3 additions & 5 deletions Source/Core/Cartographic.js
Original file line number Diff line number Diff line change
Expand Up @@ -233,13 +233,11 @@ Cartographic.equals = function (left, right) {
*
* @param {Cartographic} [left] The first cartographic.
* @param {Cartographic} [right] The second cartographic.
* @param {Number} epsilon The epsilon to use for equality testing.
* @param {Number} [epsilon=0] The epsilon to use for equality testing.
* @returns {Boolean} <code>true</code> if left and right are within the provided epsilon, <code>false</code> otherwise.
*/
Cartographic.equalsEpsilon = function (left, right, epsilon) {
//>>includeStart('debug', pragmas.debug);
Check.typeOf.number("epsilon", epsilon);
//>>includeEnd('debug');
epsilon = defaultValue(epsilon, 0);

return (
left === right ||
Expand Down Expand Up @@ -286,7 +284,7 @@ Cartographic.prototype.equals = function (right) {
* <code>false</code> otherwise.
*
* @param {Cartographic} [right] The second cartographic.
* @param {Number} epsilon The epsilon to use for equality testing.
* @param {Number} [epsilon=0] The epsilon to use for equality testing.
* @returns {Boolean} <code>true</code> if left and right are within the provided epsilon, <code>false</code> otherwise.
*/
Cartographic.prototype.equalsEpsilon = function (right, epsilon) {
Expand Down
2 changes: 1 addition & 1 deletion Source/Core/CartographicGeocoderService.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ function CartographicGeocoderService() {}
* @function
*
* @param {String} query The query to be sent to the geocoder service
* @returns {Promise<GeocoderService~Result[]>}
* @returns {Promise<GeocoderService.Result[]>}
*/
CartographicGeocoderService.prototype.geocode = function (query) {
//>>includeStart('debug', pragmas.debug);
Expand Down
4 changes: 2 additions & 2 deletions Source/Core/CesiumTerrainProvider.js
Original file line number Diff line number Diff line change
Expand Up @@ -485,7 +485,7 @@ function CesiumTerrainProvider(options) {
* When using the Quantized-Mesh format, a tile may be returned that includes additional extensions, such as PerVertexNormals, watermask, etc.
* This enumeration defines the unique identifiers for each type of extension data that has been appended to the standard mesh data.
*
* @exports QuantizedMeshExtensionIds
* @namespace QuantizedMeshExtensionIds
* @see CesiumTerrainProvider
* @private
*/
Expand Down Expand Up @@ -1201,7 +1201,7 @@ CesiumTerrainProvider.prototype.getTileDataAvailable = function (x, y, level) {
* @param {Number} x The X coordinate of the tile for which to request geometry.
* @param {Number} y The Y coordinate of the tile for which to request geometry.
* @param {Number} level The level of the tile for which to request geometry.
* @returns {undefined|Promise} Undefined if nothing need to be loaded or a Promise that resolves when all required tiles are loaded
* @returns {undefined|Promise<void>} Undefined if nothing need to be loaded or a Promise that resolves when all required tiles are loaded
*/
CesiumTerrainProvider.prototype.loadTileDataAvailability = function (
x,
Expand Down
2 changes: 1 addition & 1 deletion Source/Core/ClockRange.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
* Constants used by {@link Clock#tick} to determine behavior
* when {@link Clock#startTime} or {@link Clock#stopTime} is reached.
*
* @exports ClockRange
* @enum {Number}
*
* @see Clock
* @see ClockStep
Expand Down
2 changes: 1 addition & 1 deletion Source/Core/ClockStep.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
* Constants to determine how much time advances with each call
* to {@link Clock#tick}.
*
* @exports ClockStep
* @enum {Number}
*
* @see Clock
* @see ClockRange
Expand Down
4 changes: 2 additions & 2 deletions Source/Core/ComponentDatatype.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import WebGLConstants from "./WebGLConstants.js";
* WebGL component datatypes. Components are intrinsics,
* which form attributes, which form vertices.
*
* @exports ComponentDatatype
* @enum {Number}
*/
var ComponentDatatype = {
/**
Expand Down Expand Up @@ -137,7 +137,7 @@ ComponentDatatype.getSizeInBytes = function (componentDatatype) {
/**
* Gets the {@link ComponentDatatype} for the provided TypedArray instance.
*
* @param {TypedArray} array The typed array.
* @param {Int8Array|Uint8Array|Int16Array|Uint16Array|Int32Array|Uint32Array|Float32Array|Float64Array} array The typed array.
* @returns {ComponentDatatype} The ComponentDatatype for the provided array, or undefined if the array is not a TypedArray.
*/
ComponentDatatype.fromTypedArray = function (array) {
Expand Down
2 changes: 1 addition & 1 deletion Source/Core/CornerType.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
* @demo The {@link https://sandcastle.cesium.com/index.html?src=Corridor.html&label=Geometries|Corridor Demo}
* demonstrates the three corner types, as used by {@link CorridorGraphics}.
*
* @exports CornerType
* @enum {Number}
*/
var CornerType = {
/**
Expand Down
2 changes: 1 addition & 1 deletion Source/Core/CubicRealPolynomial.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import QuadraticRealPolynomial from "./QuadraticRealPolynomial.js";
/**
* Defines functions for 3rd order polynomial functions of one variable with only real coefficients.
*
* @exports CubicRealPolynomial
* @namespace CubicRealPolynomial
*/
var CubicRealPolynomial = {};

Expand Down
2 changes: 1 addition & 1 deletion Source/Core/EarthOrientationParameters.js
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ EarthOrientationParameters.NONE = Object.freeze({
* Gets a promise that, when resolved, indicates that the EOP data has been loaded and is
* ready to use.
*
* @returns {Promise} The promise.
* @returns {Promise<void>} The promise.
*/
EarthOrientationParameters.prototype.getPromiseToLoad = function () {
return when(this._downloadPromise);
Expand Down
Loading

0 comments on commit 85c78ed

Please sign in to comment.