Several improvements to typings #8884

thw0rted · 2020-05-29T08:33:15Z

Note up front, I had to change the command in the gulpfile from a relative path to npx. I think this is more or less universally supported now across platforms but I haven't researched it fully.

The biggest change (by volume, certainly) is moving a lot of options-argument definitions out into named interfaces with @typedef. Most of these are called ClassName.ConstructorOptions, with a couple of .LoadOptions for DataSources that have common options for a load method.
One or two commits are dedicated to converting Event to take a generic parameter specifying the callback signature. This lets you type events as e.g. Event<Function(int, bool)>, which means that in viewer.fooEvent.addEventListener((num, flag) => { .... }), the arguments are typed correctly. I only added types for the few events that I personally use but I could pretty easily fix the rest of them if you like this approach.
- I couldn't get @template to work very well so I wound up adding a hack to the gulpfile. I'm going to try to find a few minutes today to put together a tiny repro and open an issue with tsd-jsdoc to see if I am missing something. It's also possible that there just isn't any support for generic parameter defaults (Event<Listener extends Function = Function>), which is super important in this case, which would probably mean keeping the hack in place anyway.
The Resource class got a couple @typedefs that more precisely define what kind of object can be used as a queryParameter.
A couple of the changes are just for correctness -- arguments or properties that are allowed to be undefined or false, etc.
Unanswered questions are marked with *** TODO ***: a couple of properties whose shapes aren't fully documented on Entity, and one option for PolylineGraphics that can probably be a Property but I wanted to double check before making the change.
- Document shape of two properties on Entity
- Validate that PolylineGraphics#ArcType can be a Property

PS: I just noticed that I accidentally committed deleting .vscode/tasks.json, but the good news is that actually everybody should do that, because the tasks that were previously in there should now be picked up automatically via NPM integration, unless for some reason you're stuck on a year-plus-old build of VSCode.

cesium-concierge · 2020-05-29T08:33:19Z

Thank you so much for the pull request @thw0rted! I noticed this is your first pull request and I wanted to say welcome to the Cesium community!

The Pull Request Guidelines is a handy reference for making sure your PR gets accepted quickly, so make sure to skim that.

❌ Missing CONTRIBUTORS.md entry.
- Please add yourself to the contributors file!
❌ Missing CLA.
- Please send in a Contributor License Agreement (CLA) and comment back here to let us know to check this!
❔ 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.

thw0rted · 2020-05-29T08:37:59Z

I thought I'd done the CLA before but I probably forgot. (There was a whole thing about 2 years ago about having my company sign since I'm doing the work on the clock, but I finally got cleared by Legal to just sign the individual one instead.) It's done now.

thw0rted · 2020-05-29T09:59:24Z

This new commit converts exportKml to an overloaded function that returns the correct type based on the value of options.kmz. It does result in two different entries in globals.html in the docs, but I made sure the description was attached to the first entry and both have their own @example.

mramato · 2020-05-29T13:58:14Z

Yeah, I definitely thought you were a contributor already as well, either way thanks for the PR. I'm going to review now and would love to get this into Monday's release so that TS starts with it's best foot forward.

mramato · 2020-05-29T14:04:27Z

Note up front, I had to change the command in the gulpfile from a relative path to npx.

Why? I'm not a fan of npx as a concept but if there's a good technical reason for using it here instead of just calling the script directly, I'm willing to entertain the idea. Unless something has changed in node, bin scripts are guaranteed to be in the location we were using them.

OmarShehata · 2020-05-29T14:05:00Z

Can confirm we've received your individual CLA @thw0rted .

mramato · 2020-05-29T14:06:22Z

I just noticed that I accidentally committed deleting .vscode/tasks.json, but the good news is that actually everybody should do that, because the tasks that were previously in there should now be picked up automatically via NPM integration, unless for some reason you're stuck on a year-plus-old build of VSCode.

Sounds good to me, I verified that the npm scripts in VS Code picks everything up.

thw0rted · 2020-05-29T14:11:05Z

re: npx, maybe it's something I did wrong with Gulp? When I ran it, it would error that it couldn't find node_modules/.bin/tsc or whatever, and my initial assumption that it was some kind of platform-specific thing with backslash vs frontslash, etc. -- I run git-bash as my shell in this environment, not sure what yours is.

I made the change because npx works for both of us, and seemed to express intent better to me. I'm really not partial, though.

mramato · 2020-05-29T14:17:21Z

Thanks, just to debug further, were you running npm run build-ts? or were you using a global installed version of gulp cli and doing gulp build-ts? What OS are you on?

thw0rted · 2020-05-29T14:20:25Z

npm run build-ts, no global gulp, Windows 10 but in git-bash, as I said. I just tested and it looks like execSync(".\\node_modules\\.bin\\jsdoc") works directly from the node REPL, so that strongly suggests a platform separator issue. Which means either you can use path.resolve or just stick with npx.

ETA: honest question, I don't know if npx ships with all modern Node distros like npm or if I installed it myself at some point. I thought the former, but if you told me it's the latter I wouldn't be that surprised. If you know one way or another, great, but if not I guess at least everybody can use path.resolve without incident.

mramato · 2020-05-29T14:36:22Z

@thw0rted I believe npx ships with node/npm these days. I'll confirm of course. I was asking mainly so I could reproduce on my end. Also, we have other parts of gulpfile where we do the same thing so ultimately we should make them consistent. Assuming npx is always available, I'm not going to bikeshed it any further and will just look into it as a followup PR. Thanks.

mramato · 2020-05-29T14:40:41Z

Source/Core/Resource.js

+ * Initialization options for the Resource constructor
+ *
+ * @property {String} url The url of the resource.
+ * @property {Resource.QueryParameters} [queryParameters] An object containing query parameters that will be sent when retrieving the resource.


QueryParameters is not showing up properly in the doc, hopefully an easy fix.

I went ahead and added a one-liner so it's not blank, at least. If you want to flesh it out feel free.

I think it's going to be really confusing for non TS users reading the doc to understand what they are supposed to pass here. What is the fundamental issue you are trying to prevent here, people passing an object that has more than just primitive values? Technically users can pass anything that has a toString function, right? So wouldn't Object still be the best option here no matter what?

mramato · 2020-05-29T15:05:17Z

Source/Core/Event.js

@@ -46,7 +47,7 @@ Object.defineProperties(Event.prototype, {
 * An optional scope can be provided to serve as the <code>this</code> pointer
 * in which the function will execute.
 *
- * @param {Function} listener The function to be executed when the event is raised.
+ * @param {Listener} listener The function to be executed when the event is raised.


This ends up creating a non-existent Listener entry in the doc:

I started to dig into template/generics a little on the JSDoc and I agree they don't appear to work at first glance. I'll see what I can find. I want to be careful about trying to do too much too fast since we want to use generics in more places (like Property) but I don't want to get too crazy with workaround either.

I totally forgot to link you to englercj/tsd-jsdoc#134 -- it looks like @template is flat-out broken, but also there isn't consensus about how to provide a default value for a type parameter. Sadly that probably means the ugly hack in the gulpfile will be required for the foreseeable future if you're going to use generics.

mramato · 2020-05-29T15:05:47Z

Source/Core/Math.js

@@ -429,7 +427,6 @@ CesiumMath.DEGREES_PER_RADIAN = 180.0 / Math.PI;
 *
 * @type {Number}
 * @constant
- * @default {@link CesiumMath.RADIANS_PER_DEGREE} / 3600.0


I'm okay with these changes, but it made me realize there's a bug with Math/CesiumMath in my PR, so thanks!

I'm not positive this would work even without the Math error, because I can't find anything saying that an expression (not a literal) for @default is supposed to be supported.

mramato · 2020-05-29T15:14:51Z

Source/DataSources/DataSource.js

   */
  errorEvent: {
    get: DeveloperError.throwInstantiationError,
  },
  /**
   * Gets an event that will be raised when the value of isLoading changes.
   * @memberof DataSource.prototype
-   * @type {Event}
+   * @type {Event<function(this, boolean)>}


I was worried about how JSDoc would handle this, but I think this is okay, assuming we can get Event as a template working a little better.:

However, I'm pretty sure this breaks duct typing because CustomDataSource/CzmlDataSource/KmlDataSource/GeoJsonDataSource don't have the same signatures.

I'm not sure if the best course of action is to update the signatures, or pull out the Event related changes in a separate PR for more discussion. I'll finish up the rest of the review and think about it.

FWIW I have lines like

let src: DataSource; ... src = new KmlDataSource(...)

in my code and it didn't break when compiling against this branch's typings. I can see where you'd be concerned, though, so I understand if you want to hold off. (If there's no rush, I could always fix all uses of Event on Monday, it's not actually that many.)

mramato · 2020-05-29T15:40:03Z

Source/DataSources/Entity.js

+ * @property {Boolean} [show] A boolean value indicating if the entity and its children are displayed.
+ * @property {Property | string} [description] A string Property specifying an HTML description for this entity.
+ * @property {PositionProperty | CallbackProperty | Cartesian3} [position] A Property specifying the entity position.
+ * **** TODO ****


What are these TODOs for? Also, CallbackProperty is officially supported here. The object needs to conform to the PositionProperty signature to work in all cases. Please write up a separate issue so that we can either add a CallbackPositionProperty or something similar.

Sorry, it's asking about the type of orientation and viewFrom. Are they Matrix4? Cartesian3? The docs aren't much help, as they just use the term to define itself ("Gets or sets the orientation" -- gee, thanks :D )

re CallbackProperty, was that isn't officially supported? I've been using it for a while but if that's wrong, we can remove it from the typing here and I'll just any-cast and cross my fingers until official support gets added.

viewFrom is a Cartesian3. Orientation is a Quaternion

There are places where we call PositionProperty.getValueInReferenceFrame, such as path visualization and exportKml. So CallbackProperty is technically incorrect for now. Turns out we have an issue for this already: #5534 so let's revert for now and maybe we can fix for 1.71.

mramato · 2020-05-29T15:42:29Z

Source/DataSources/EntityCluster.js

@@ -963,7 +963,8 @@ EntityCluster.prototype.destroy = function () {
 * @callback EntityCluster.newClusterCallback
 *
 * @param {Entity[]} clusteredEntities An array of the entities contained in the cluster.
- * @param {Object} cluster An object containing billboard, label, and point properties. The values are the same as
+ * @param {{billboard: Billboard, label: Label, point: PointPrimitive}} cluster An object


I get the intent here but this just ends up being an any in the type definition either way.

I totally didn't notice that. Was I right about the actual shape of the object? I can fix it to use the correct @param syntax, I'm just spoiled by the shorthand TS lets you use.

Yes, the shape is correct.

mramato · 2020-05-29T15:45:26Z

Source/DataSources/PolylineGraphics.js

+ * @property {MaterialProperty | Color} [material=Color.WHITE] A Property specifying the material used to draw the polyline.
+ * @property {MaterialProperty | Color} [depthFailMaterial] A property specifying the material used to draw the polyline when it is below the terrain.
+ *
+ * **** TODO **** this was originally only ArcType, not a Property (!) -- pretty sure that was wrong?


Yep, it's definitely a Property in the code, so you can remove this todo and keep your change. Thanks!

mramato · 2020-05-29T15:48:06Z

Source/DataSources/PropertyBag.js

- *
- * @param {Object} [value] An object, containing key-value mapping of property names to properties.
- * @param {Function} [createPropertyCallback] A function that will be called when the value of any of the properties in value are not a Property.
+ * @typedef {PropertyBagType}


This breaks JSDoc HTML output completely.

mramato · 2020-05-29T15:49:16Z

Source/DataSources/exportKml.js

@@ -221,6 +221,33 @@ IdManager.prototype.get = function (id) {

  return id.toString() + "-" + ++ids[id];
 };
+/**
+ * @variation 2 KML return


Changes in this file break exportKml JSDoc output completely.

Well this is weird. The version I'm looking at right now is not what I committed. I saw that there's some kind of commit hook to run a prettifier -- could that be the culprit? It's important to have

/** * Body of documentation * * @variation 1 * @param ... * @example (for variation 1) *//** * @variation 2 * @param ... * @example (for variation 2) */

Exactly thus, with no spaces between the closing tag for the first block and the opening tag for the second. That's part of the JSDoc spec, apparently, though ironically documentation about the spec is a bit dodgy in my experience. Is it possible to exempt a section or a whole file from being "massaged" by the prettifier?

With the blocks aligned correctly, you get this:

Each overload gets its own entry in globals.html but I only put a body in one of them, so it seems pretty clear IMHO. If you'd rather keep one body then we're probably stuck with the union return.

I think I would rather stick with the union return for now. This type of decision may affect the API elsewhere and our main goal is keeping the API consistent. I would just revert the file for now. Feel free to open a separate PR for further discussion.

mramato · 2020-05-29T15:52:58Z

Source/Widgets/CesiumWidget/CesiumWidget.js

@@ -128,8 +128,8 @@ function configureCameraFrustum(widget) {
 * @param {Clock} [options.clock=new Clock()] The clock to use to control current time.
 * @param {ImageryProvider} [options.imageryProvider=createWorldImagery()] The imagery provider to serve as the base layer. If set to <code>false</code>, no imagery provider will be added.
 * @param {TerrainProvider} [options.terrainProvider=new EllipsoidTerrainProvider] The terrain provider.
- * @param {SkyBox} [options.skyBox] The skybox used to render the stars.  When <code>undefined</code>, the default stars are used. If set to <code>false</code>, no skyBox, Sun, or Moon will be added.
- * @param {SkyAtmosphere} [options.skyAtmosphere] Blue sky, and the glow around the Earth's limb.  Set to <code>false</code> to turn it off.
+ * @param {SkyBox| false} [options.skyBox] The skybox used to render the stars.  When <code>undefined</code>, the default stars are used. If set to <code>false</code>, no skyBox, Sun, or Moon will be added.


If we're changing this here, we should change it for the other params in this signature as well, such as ImageryProvider

mramato · 2020-05-29T16:26:14Z

@thw0rted thanks again for the PR! The Entity and GraphicsXXX changes are all fine with me.

The ImageryProvider changes are okay as well, but we really should do them all for consistency. Some are still using the "old" documentation strategy, such as TileMapServiceImageryProvider, IonImageryProvider, etc.. It's easy to check them all by going down the @see list in ImageryProvider.js. If you are up for this, great. Otherwise I can take care of it after we merge this since they are mostly compatible.

The main thing stopping me from merging this is the broken JSDoc and Event and PropertyBag changes. I would recommend backing them out and opening a separate PR. I don't care about cleaning up the commit history, so this should be trivial to do. We may still be able to get them in 1.70, I just want to get as much as we can into the primary PR ASAP.

Since we want to get this into 1.70, I'm also okay with taking this PR over if you are unable to work on it until Monday. Just let me know.

Thanks again, now you'll go down in history as the first community member to contribute to our official TS definitions!

thw0rted · 2020-05-29T16:39:04Z

Please go ahead and back out the things you're not comfortable with. It's quittin' time here in Europe for today, and while I might put in a few hours over the weekend, I don't want to commit to it ahead of time or hold up your release.

mramato · 2020-05-29T17:30:58Z

Thanks @thw0rted. I'm not sure why I thought you were in the states, enjoy your weekend.

mramato · 2020-05-30T04:46:17Z

Thanks @thw0rted!

I submitted some clean-up and backed out some changes that we need to think about more, but I parked them on a branch, thw0rted-bonus, and definitely plan on revisiting them ASAP.

thw0rted added 9 commits May 28, 2020 14:19

Give up on trying JSDoc tags for PropertyBag

3172bc4

Example of exporting ConstructorOptions interface

f30a9b0

Break out constructor options: Entity and Graphics

6dd9e0c

Various small type fixes

ca986af

Selected/tracked entity can be undefined

9678a43

First pass at generic Events

14c5ec1

Constructor interfaces for ImageryProviders

2b91930

Load-method option interfaces for some DataSources

1b16c9a

Options interfaces for Viewer, Resource

a89f7c9

thw0rted mentioned this pull request May 29, 2020

Generate official TypeScript type definitions #8878

Merged

12 tasks

thw0rted added 2 commits May 29, 2020 10:40

Add CONTRIBUTORS entry

e18535f

Use function overloading for exportKml

d3cf0ac

mramato reviewed May 29, 2020

View reviewed changes

Spell out docs for Resource.QueryParameter

d734c83

mramato reviewed May 29, 2020

View reviewed changes

Undo some changes

a38104e

mramato added a commit that referenced this pull request May 29, 2020

Re-add changes from #8884

9dce2f3

Add ConstructorOptions to the rest of the ImageryProviders

dca5c58

mramato merged commit adc772c into CesiumGS:typescript-definitions May 30, 2020

thw0rted pushed a commit to thw0rted/cesium that referenced this pull request Jun 1, 2020

Re-add changes from CesiumGS#8884

c0c5c52

thw0rted pushed a commit to thw0rted/cesium that referenced this pull request Jun 2, 2020

Re-add changes from CesiumGS#8884

03d67d7

thw0rted pushed a commit to thw0rted/cesium that referenced this pull request Jun 3, 2020

Re-add changes from CesiumGS#8884

ca06c12

thw0rted pushed a commit to thw0rted/cesium that referenced this pull request Jun 5, 2020

Re-add changes from CesiumGS#8884

bb0491d

thw0rted pushed a commit to thw0rted/cesium that referenced this pull request Sep 21, 2020

Re-add changes from CesiumGS#8884

5477ded

thw0rted pushed a commit to thw0rted/cesium that referenced this pull request Nov 6, 2020

Re-add changes from CesiumGS#8884

0175ea0

thw0rted pushed a commit to thw0rted/cesium that referenced this pull request Nov 6, 2020

Re-add changes from CesiumGS#8884

2a78bca

thw0rted pushed a commit to thw0rted/cesium that referenced this pull request Jul 20, 2021

Re-add changes from CesiumGS#8884

15f7653

Several improvements to typings #8884

Several improvements to typings #8884

Conversation

thw0rted commented May 29, 2020 • edited Loading

cesium-concierge commented May 29, 2020

thw0rted commented May 29, 2020

thw0rted commented May 29, 2020

mramato commented May 29, 2020

mramato commented May 29, 2020

OmarShehata commented May 29, 2020

mramato commented May 29, 2020

thw0rted commented May 29, 2020

mramato commented May 29, 2020

thw0rted commented May 29, 2020 • edited Loading

mramato commented May 29, 2020

Choose a reason for hiding this comment

thw0rted May 29, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thw0rted May 29, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mramato commented May 29, 2020

thw0rted commented May 29, 2020

mramato commented May 29, 2020

mramato commented May 30, 2020

thw0rted commented May 29, 2020 •

edited

Loading

thw0rted commented May 29, 2020 •

edited

Loading

thw0rted May 29, 2020 •

edited

Loading

thw0rted May 29, 2020 •

edited

Loading