More typescript fixes

[thw0rted]

Make Event generic and add a callback signature in a few places

There are a lot more places this could be used but it should be non-breaking and just allows for future refinement.

ConstructorOptions for Resource

I can't remember why this one got taken out?

exportKml overload actually works

The catch is that I had to exclude it from prettier until they address prettier/prettier#7724. The overloads do show up separately in the docs but I think it looks OK that way.

Another stab at making PropertyBag actually work

I pinged jsdoc/jsdoc#1285 for an update. All the SO questions, etc, that I can find about how to describe something like PropertyBag (a class with its own methods and properties, and also an index signature for "other" properties) say the only option is an intersection type, which JSDoc refuses to support on the basis that it's not part of Closure Compiler. (The questions actually all say "this is an antipattern, don't do this in the first place" but it's probably too late to change?)

I think the hack I came up with will work for now. Basically, the class is called PropertyBag so that it gets the right name during generateDocumentation, but for Typescript purposes it's renamed to PropertyBagBase with a regex replace, then a top level type alias is declared using union types.

The one thing I wasn't able to do is figure out how to document in JSDoc that PropertyBag has an index signature. Is there a more Javascript-y name for "index signature"? I think I would have called it the "index operator" back before I learned TS, but I can't find anything online using either name. This isn't a new problem -- the docs have never actually given an example of using e.g. ent.properties.someProperty -- but since I was in there anyway I was looking to improve it. If you can find the right tag to use, by all means let's add it.

[cesium-concierge]

Thanks for the pull request @thw0rted!

• ✔️ 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.

[thw0rted]

I just noticed that these typings break my build, not because of any changes I made, but because I rebased them on top of master which added the properties defaultDayAlpha and defaultNightAlpha to ImageryProvider but did not add them to the implementations of that interface. Your interim solution of adding the necessary properties to each implementation fails to catch when a new property is added to the interface but missed in one (or all) of the implementations, unfortunately.

I think in the big PR from last week there was some discussion about why these classes don't use inheritance, but I didn't see any mention of the @extends tag. As an experiment, I added @extends ImageryProvider to WebMapServiceImageryProvider and it did not appear to immediately explode. VSCode complains because @extends belongs on a "real" class, not a class-like function, but generateDocumentation and build-ts both seem happy with it. Would you be OK with adding @extends to all the implementations of ImageryProvider to avoid this problem?

[mramato]

exportKml overload actually works

I'm not sure what you mean by this. How is it actually broken in master?

[mramato]

I just noticed that these typings break my build, not because of any changes I made, but because I rebased them on top of master which added the properties defaultDayAlpha and defaultNightAlpha to ImageryProvider but did not add them to the implementations of that interface. Your interim solution of adding the necessary properties to each implementation fails to catch when a new property is added to the interface but missed in one (or all) of the implementations, unfortunately.

This is fixed via new smokescreen tests in #8908. If a new ImageryProvider is added, we'll need to add it to the test, but that is not a common occurrence and should just be part of the review.

[mramato]

Would you be OK with adding @extends to all the implementations of ImageryProvider to avoid this problem?

We're not quite ready to do it yet (since there are lots of places, not just ImageryProvider) where we would want to do this; but it is up for discussion. I'm going to start an issue on it soon.

[mramato]

It's a little weird that Listener shows up in the JSDoc here as a Type without any further context:

Not the end of the world, but I wonder if we're doing something wrong or if there's a better way.

[mramato]

I can't fine @template in the official JSDoc documentation, https://jsdoc.app/.

It this not actually valid JSDoc and instead a Closure compiler extension? (as mentioned here https://github.com/google/closure-compiler/wiki/Annotating-JavaScript-for-the-Closure-Compiler#template-t)

Also, the extends Function = Function thing working almost looks like a happy accident.

I'm still learning my way around TS generics, but does this basically let non-templated versions of Even still work with a default Function callback that takes any number/type of arguments?

[mramato]

@kring meant to CC you here because I'd like to hear your thoughts on this as well, since there are lots of places we'll want to make use of generics if we can.

[kring]

Ideally the Event class's definition would look like this:

export class Event<Listener extends (...args: any[]) => any = (...args: any[]) => any> {
    constructor();
    readonly numberOfListeners: number;
    addEventListener(listener: Listener, scope?: any): Event.RemoveCallback;
    removeEventListener(listener: Listener, scope?: any): boolean;
    raiseEvent(...arguments: Parameters<Listener>): void;
}

Using (...args: any[]) => any is better than Function because it actually has a call signature, unlike Function. So you can't use Function with Parameters<Listener> to get the nicely-typed signature for raiseEvent.

I just pushed up a change to this, and it seems to work ok. Even the generated reference doc looks pretty good, even if we're abusing it a bit. But let me know if I've missed something.

[thw0rted]

You're right, @template is a Closure extension -- look at the very bottom of the tsd-jsdoc README. That's also the bit where it said you need the plugin to make it work. There are also a bunch of different competing syntaxes for function signatures: Function.<number, boolean>, function(number):boolean, (n: number) => boolean. I'm not sure which of those is "normal" JSDoc, which is comes from Closure, etc. I know Function (without constraints) is fine for everybody.

I believe in my experience that the TS compiler treats Function and (...args: any[]) => any as equivalent, sort of like any vs {[key: any]: any}, though there may be some nuance I'm not familiar with. A more specific call signature, like (n: number, b: boolean) => void should be assignable to either. Also, these are event listeners, so I believe the return type should always be void -- there's no way to do anything with the function result, right?

I actually originally wanted to make Event generic on <...args: any[]>, which would make typing raiseEvent easier, but generic rest parameters are a relatively recent Typescript addition and I can't even begin to imagine how you'd document them as valid JSDoc. Full disclosure: I wasn't too worried about raiseEvent because I didn't expect users to be raising events, just consuming them.

[kring]

Function isn't quite the same in that you can't use it with Parameters<T>. This TypeScript playground shows what I mean:

https://www.typescriptlang.org/play/index.html#code/MYGwhgzhAECiBuBTAdgFwPICcCWBzbyYIAPADLYSoqKbSIAeVyAJjAGICuywq2A9smgBeaJ268BAPmgBvAFDRF0MM2YIUqcpWqYAFCApMaALmhajmADTQIwPgAdEAflNhkATwCUp+H2zNZOQUlAF9gxUwwCkR1NF0AOkSwTFwIUwAFZLAAW0QqTAgyQx1Jb2hff0Dw6DCwuVBIGFjUAFk+ZmwAM2xEZiLtZBo6RhRWaASklLTlDwBtAF1PYWk3d2FxxPjk1Nc5xeWZ92l5JWVVZvMdfWLBzFNL2+tbB2ddrx8-APlqsNPI6OaEy2UwyWVy+UKDxopQ+lW+p1qQVQ7kc0Ba7jEwHWunopmQHGyACMaNZ3KZKDhkLglkJpPiiTQANxBOzISh0daDADucCQaDaHW6vWI6Mxkl0nmZiC25z5mhuNF0OLxBOJVmgZJsqEp1IO9ElcmlKjUcqhemV0Hpapp0n1Uvi-wgMTlugALNYAEQAZg9BqNso0ZqVuK1Opt0DthodUSdgI9VEoHusAEYDUA

Also note that nice static typing on raiseEvent with my new version, even though you're right that most CesiumJS users won't be raising events. The last two lines in the TS playground are intentionally errors to show that the calls are type checked correctly.

It all seems to work ok with tsd-jsdoc and with jsdoc itself, unless you spot a problem I didn't.

[kring]

Also, these are event listeners, so I believe the return type should always be void

Yeah that's a good point. Certainly raiseEvent ignores the return values and returns void itself. No major harm in the listeners returning non-void, but it's a little misleading.

[thw0rted]

there may be some nuance I'm not familiar with

You found it! 😁

Seriously though, this is great, as long as we don't wind up with some kind of issue with other consumers of the docs -- like I said, I'm not really clear on who wants function vs Function vs Function.<T,U,V> vs () => Whatever and I was worried that the arrow-notation version was "too Typescript-y". If the team is happy with it, I certainly am too.

[mramato]

I'm not sure why this is needed, especially since there are a lot of other instances of queryParmeters in the code that just use Object. It leads to confusing JSDoc

Since we ultimately just toString the values, I'm not sure why a special type is needed.

[thw0rted]

I think maybe the previous branch had updated some of the other references, but I lost track of it when these changes were backed out? Anyway, it's been a minute, but I think this #6205 (comment) might be the reason why, at least on my own typings, I wanted a more constrained type. I thought that issue ({x: [1,2,3]} turning into x=1&x=2&x=3 instead of x=1,2,3) had actually been resolved, which would mean that the values are not simply toString'd. Could be mistaken? As a later comment in that thread points out, Resource isn't really "for" consumers anyway.

(What really should be getting passed here is URLSearchParams but I get that you still have legacy support to consider and it's almost certainly not worth the weight of a polyfill.)

[mramato]

Just to be sure, I did some testing and:

var resource = new Resource({
  url: "http://test.invalid/test",
  queryParameters: {
    key1: ["value1", "value5", "value6"],
    key2: "value2",
  }
});

Gets turned into http://test.invalid/test?key1=value1&key1=value5&key1=value6&key2=value2

And

var someObject = { toString: () => "bob" };
var resource = new Resource({
  url: "http://test.invalid/test",
  queryParameters: {
    key1: someObject,
  },
});

Gets turned into: http://test.invalid/test?key1=bob

So toString is definitely called, though it's smart enough to loop over arrays and add the key multiple times. While I appreciate the desire to more clearly define the types, I think having a marker object in the JSDoc HTML is confusing. We would also need to update the other places where queryPamaters is used, such as setQueryParamaters. I'd rather just back the QueryParameters type altogether and use object. We can certainly keep the Resource options object you introduced.

If you feel strongly about this, please open a separate issue specifically to discuss QueryParameters, I don't want to see this specific piece of code hold up the rest of the PR.

Thanks!

[thw0rted]

I think I originally created that type in response to the issue I linked above, more as a hint to myself than anything. Please go ahead and back it out and if I feel the need to place some constraint for myself, I will.

[thw0rted]

The key word in the line about exportKml is "overload". I wrote an overload, which is two doc-blocks with @variant tags "nestled" so that you wind up with *//** between them, but Prettier quietly broke them during commit due to prettier/prettier#7724 so you backed that out. I put it back along with a Pretty ignorefile entry and it works as an overload again.

Re: Listener showing up as an unreferenced type, would it be clearer if it were simply renamed to something more specific like "EventCallback" or something, and maybe there was a note describing it in the class description? Alternately, does the AST from the JSDoc parser include a node for generic type parameters that maybe needs to be added to the doc template? If not, that sounds like an issue to file with jsdoc.

[mramato]

The key word in the line about exportKml is "overload".

Sorry if I'm confused here, but I just don't get why this is needed at all. From my understanding as it's documented the result can either be exportKmlResultKml or exportKmlResultKmz. If this is somehow a problem, I think I would rather just declare a single exportKmlResult and have it have all 3 properties. You can check the existence of the kml or kmz properties to see which was generated. This is a lot cleaner to me than duplicating the doc here. I realize I'm a bit green on TS here so if you could help me understand I would appreciate it.

[thw0rted]

You can document exportKml with {boolean} options.kmz, and return type Promise<result1|result2>, then the consumer has to cast the result even though they know which one they asked for. In Typescript, you'd solve this with a function overload signature. Functions can only have one body but any number of signatures, and they're matched in the order declared.

function lookup(key: "strThing"): string;
function lookup(key: "numThing"): number;
function lookup(key: string): string | number | Array { ... }

lookup("strThing"); // types as string
lookup("numThing"); // types as number
lookup("otherThing"); // can't tell at compile time, types as string | number | Array

tsd-jsdoc will emit this but only if you have two "nestled" doc blocks describing the same function. So I created the double block such that passing options.kmz = true will only return Promise<kmzResult> and if it's false or not specified it will be a KML result.

Basically, it makes the docs a bit bigger but it saves users from having to cast the result. I think this is preferable but I understand it has some drawbacks. (You should see the declaration for document.createElement in the DOM lib file!) Anyway, feel free to back it out but I thought this was friendlier to consumers.

[mramato]

@thw0rted can you please merge in master, this will bring in the TS smokescreen changes and some other cleanup. Thanks!

[thw0rted]

I just rebased and am running build-ts and generateDocumentation. I haven't tried the new smoketests or anything else yet.

Everything seems fine except for my PropertyBag hacks. As mentioned in the OP, the consensus seems to be that supporting arbitrary properties (index signature) on a class is an antipattern and it's tough to describe properly in TS. Either you make an any index signature so that the return of an index operation is not type-safe, or you use a union type and make it impossible to actually instantiate the "class" (because it's not just a class anymore) from Typescript.

In my typings, as in this PR, I opted for the latter, because I don't expect end users to make a new PropertyBag() -- more likely it would be new Entity({..., properties: {a: 1, b: true}}). If I'm right, it's more important for myEnt.properties.someProp to type correctly. I could totally be missing some common use case, though.

Anyway, I pushed one more commit that "fixes" the smoke test by not actually instantiating a PropertyBag, and instead doing a cast to make TS think an assignment is happening. If you're happy with this, great, and if not we can back out all the PropertyBag related stuff.

[bkuster]

dont know if I'm too late to the party. Found that constructing the CesiumWidget with imageryProvider: false throws my typings off. Removing some whitespaces in the jsdoc ({ImageryProvider | false} to {ImageryProvider|false} solves this for me. Can we get this in here too? otherwise I'll open another PR

[kring]

@thw0rted regarding exportKml, even though I agree with you that overloads in real TypeScript would work well here, I'm inclined to agree with Matt that the cost outweighs the benefit in this case. The resulting reference docs are pretty confusing. exportKml shows up twice (including in search results), and it takes a lot of reading to figure out why and what the difference between the two is. And the benefit in client code is relatively small relative to Matt's suggestion of returning a unified ExportKmlResult; it only avoids a single undefined check of the property you expect to be returned.

Perhaps the best design would be to have a separate exportKmz function, even if it's just a one-liner that calls a common implementation. We'd have to add a deprecationWarning on options.kmz and continue to support it for a couple of releases.

[kring]

Also please mate don't force push. You accidentally clobbered some of my changes. I'll re-add them.

[kring]

Do we want this change? I understand it's hard to prevent this assignment via the type system, but it's not going to work at runtime because expected properties like propertyNames will be missing.

[kring]

Oh I see, PropertyBag can't be constructed anymore because it's a type. Pretty sure that's a problem. It may not be common to construct PropertyBag, but it happens. TerriaJS does it.

[kring]

Hi @bkuster,

dont know if I'm too late to the party. Found that constructing the CesiumWidget with imageryProvider: false throws my typings off. Removing some whitespaces in the jsdoc ({ImageryProvider | false} to {ImageryProvider|false} solves this for me. Can we get this in here too? otherwise I'll open another PR

I don't quite understand why the whitespace would throw off the typings. The generated Cesium.d.ts looks fine to me, and certainly TS itself isn't sensitive to whitespace there. I'm not opposed to that change just for the sake of tidiness, but it could be worth double-checking there isn't something else going on there.

[bkuster]

@kring yeah, must have been some freaky thing over my end, all fine now (probably forgot to rebuild the TS after pulling from master or similar), sorry for the noise.

[thw0rted]

Kevin, sorry about clobbering your changes -- I'm used to working in my own "private" branches where rebase-and-force is a safe workflow. I need some practice working with a shared upstream!

You're right that the docs are pretty unwieldy for exportKml and the benefit isn't huge. It would be nice if we had an option for marking one as "don't emit in the regular docs template, but do emit in the TS typings". Probably not possible? Maybe it would make sense to update the docs template to ignore anything that has an @variant value other than 1? I strongly suspect it's not used anywhere else at present.

Re: PropertyBag, I'm going to do a bit more research and see if there's not some way to make a declaration that's a) new-able, b) has strongly-typed methods and explicit properties, and c) also has a strongly typed index signature. I suspect there isn't, so we'll wind up having to give the index signature an any-type.

ETA: if anybody wants to follow microsoft/TypeScript#38977, feel free, but I'm not holding my breath.

[kring]

Kevin, sorry about clobbering your changes

No worries! :)

It would be nice if we had an option for marking one as "don't emit in the regular docs template, but do emit in the TS typings".

Yeah that'd be cool. Again, though, I'm not really sure it's worth the cycles. Even in the best-case scenario it still forces to maintain duplicated doc, for very minimal benefit.

[cesium-concierge]

Thanks again for your contribution @thw0rted!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

[cesium-concierge]

Thanks again for your contribution @thw0rted!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

[cesium-concierge]

Thanks again for your contribution @thw0rted!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

[thw0rted]

Sorry to be away from this for so long. I just rebased back to master (I did check here first to make sure I wouldn't clobber anybody!) and am going to see if I can rework this, possibly into multiple PRs.

[thw0rted]

I think there were only one or two of these functions that the compiler actually noticed and complained about, but I figured while we're in there it would make sense to just mark everything.

[cesium-concierge]

Thanks again for your contribution @thw0rted!

No one has commented on this pull request in 90 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 90 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

[cesium-concierge]

Thanks again for your contribution @thw0rted!

No one has commented on this pull request in 90 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 90 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

[thw0rted]

I just created a new PR, #10292, which has all the important changes from here without any of e.g. Promise<xyz> -> Promise.<xyz> -- those were a holdover from the when.js days. @sanjeetsuhag maybe you could take a look at that one if you have time? I believe it addresses all the comments from above, the only outstanding thing would be that it adds three TODO comments. It would be up to you whether they should be a new issue, passed to a team member to answer, or handed back to me to finish before closing out the PR.

[ggetz]

@thw0rted Can we close this in favor of #10292 (review)? Even closed, this PR will still exist for the sake of documenting discussion.

[thw0rted]

Fine by me. I'll try to take a look at your comments over there today -- thanks for taking the time to look it over!