[WIP]: Improve jsdoc tags for tsd-jsdoc integration

[bampakoa]

This PR is a continuation of #6767 . It contains improvements on the jsdoc block tags so that the output from tsd-jsdoc will be more accurate. Please note that the PR refers to the Source directory only.

• Core
• DataSources
• Renderer
• Scene
• Widgets
• Workers

As this PR is going to be a long one, I would really appreciate if anyone from the Cesium team can review along the way.

[cesium-concierge]

Thanks for the pull request @bampakoa!

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

Reviewers, don't forget to make sure that:

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

---

I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome.

🌍 🌎 🌏

[hpinkos]

Looks good so far @bampakoa! Just ping me when this is ready for another look

[bampakoa]

Reverted changes to ApproximateTerrainHeights since we are not going to include private artifacts in the typings file and Intersect because it looks like an enum (we need to check with tsd-jsdoc for enum support first).

@hpinkos

• There are many cases such as Intersect which look like an enumeration. Will we be able to change documentation and declare it as enum block tag as soon as tsd-jsdoc supports creation of enumeration types?
• I have recenly came across a type of Promise.<undefined>. Is this a typo?

[hpinkos]

1. Yes, I think it's okay to add the @enum tag to our enumerated types. They still show up fine in the generated documentation.

2. We have Promise.<type> in documentation to note what the promise will return when it resolves. Promise.<undefined> means that promise will resolve but doesn't resolve to a value. Is that causing errors for you?

[bampakoa]

I think it is ok to define it as Promise only. What do you think?

[hpinkos]

@bampakoa yeah that sounds fine. We don't do @returns undefined and that's a similar idea

[bampakoa]

@hpinkos noticed that in some cases an object has functions defined in its namespace such as ComponentDataType and other as properties of the object such as PrimitiveType. Is this by design? Could you please elaborate a bit on that choice?

[hpinkos]

@bampakoa these are all helper functions related to those enums. It's just taking advantage of how flexible javascript is for the convenience of keeping related things grouped together.

[bampakoa]

Sure, but why methods are not part of the enum in the ComponentType?

[hpinkos]

There's no difference between

var enum = {
    value: 0, 
    myFunction: function() { }
};

and

var enum = { value: 0 };
enum.myFunction = function() {};

The result object is the same. Does it make a difference from a documentation standpoint?

[bampakoa]

Hard to say yet because tsd-jsdoc does not support enums yet. Let me check and I will get back to this. Nevertheless, I was just wondering why you wrote it in a different way than the others 😃 . The first approach is more performant in some engines such as V8.

[bampakoa]

@hpinkos I checked with tsd-jsdoc and they support enum block tags. But I discovered that there is no need to convert them after all, since they actually return an object using freezeObject method.

With that being said, methods that return or have a parameter of such an enumeration should reference the value of the enumeration and not the enumerated object itself. For example, the following:

@returns {Intersect}

should be:

 @returns {Number} 
 @see Intersect

What's your opinion on that?

I did not push any commit because there are a lot of such occurences and would like to be sure about that first 😉

[hpinkos]

I'm honestly not too sure about that one. Does anyone else have an opinion on that? @mramato?

[mramato]

My initial reaction is that tsd-jsdoc needs to be updated to support enums, TS has full enum support already, right? So this seems like a tool issue to me, not a Cesium issue.

[bampakoa]

@mramato they have indeed support for enum block tags. They suggested that we should refer to the actual value of the enumeration and not the enumeration object by itself. For example, in a function that returns Intersect.INSIDE, we should document that it returns a Number and not the Intersect object. You can see the full discussion here englercj/tsd-jsdoc#49

[mramato]

I'm not super familiar with TypeScript, but if we were writing typescript manually wouldn't we document it as returning the enum rather than a number? That's how other doc systems, like C#, work.

[bampakoa]

@mramato @hpinkos thanks very much for your comments. After searching about the issue, I made some second thoughts. I think that we do not need to document it as enum since then it will be a documentation of the actual implementation. We care only about the type safety of the object. Thus, my suggestion is to document it like typedef:

* @typedef Intersect
* @type {Object}
* @property {Number} OUTSIDE Represents that an object is not contained within the frustum.
* @property {Number} INTERSECTING Represents that an object intersects one of the frustum's planes.
* @property {Number} INSIDE Represents that an object is fully within the frustum.
*/
var Intersect = {
   OUTSIDE: -1,
   INTERSECTING : 0,
   INSIDE : 1
};

That way, it is handled properly from functions that use this type:

/**
 * Determines which side of a plane the oriented bounding box is located.
 * @param {Plane} plane The plane to test against.
 * @returns {Intersect} {@link Intersect.INSIDE} if the entire box is on the side of the plane
 *                      the normal is pointing, {@link Intersect.OUTSIDE} if the entire box is
 *                      on the opposite side, and {@link Intersect.INTERSECTING} if the box
 *                      intersects the plane.
 */
intersectPlane(plane: Plane): Intersect;

[hpinkos]

@bampakoa yes, I think that's a good solution. Can you push that up? I want to make sure our reference documentation still generates something reasonable with that change.

(if you didn't know already, you can run npm run generateDocumentation to build the reference doc locally)

[bampakoa]

@hpinkos sure. Do you want me to push only for Intersect so that you can check?

[hpinkos]

Yes, that works. If you've already changed it everywhere feel free to push that up, but just Intersect will be good for testing

[bampakoa]

Nope I have not, because I wanted to check first with you about any problems.

Let me push Intersect for you and if it is ok, I can use the generateDocumentation command to check all the other by myself.

[bampakoa]

@hpinkos the commit for Intersect is ready for review.

[hpinkos]

It looks like Intersect is no longer listed in the ref doc after this change

[hpinkos]

I think you need the @exports

[bampakoa]

@hpinkos yeah, I had removed it for testing purposes and never restored it. Sorry about that. I have pushed the change in the PR. Can you please confirm?

[bampakoa]

@hpinkos just to ping you in case you have missed my previous comment. I would appreciate your feedback on the missing exports block tag prior to push any further changes. Cheers 😃

[hpinkos]

Thanks for following up! I did miss that comment from a few weeks ago.

I pulled down your branch and ran generateDocumentation and I still don't see Intersect listed in the ref doc.

[bampakoa]

Can you describe how can I check ref doc listing by myself so that I can perform some tests without bothering you?

[hpinkos]

Yes, from the cesium root directory:

1. Run npm run build (you only need to do this once if you haven't already)
2. Run npm run generateDocumentation (you'll need to run this to see changes you've made to the doc)
3. Run node server.js (starts a local server)
4. Browse to localhost:8080 and you'll see a page like this:
   
5. Click the Documentation link to bring up the ref doc. Make sure any changes you make have roughly the same result as what's on our hosted ref doc: https://cesiumjs.org/refdoc/

[bampakoa]

@hpinkos It is now in http://localhost:8080/Build/Documentation/global.html#Intersect because of the typedef block tag. As far as jsdoc is concerned, this is normal behavior but I am not sure if this is valid in Cesium context.

[bampakoa]

Hey, any update on this?

[hpinkos]

I'm honestly not sure. @mramato do you have an opinion on #6899 (comment)?

[bampakoa]

@hpinkos @mramato what do you think about leaving enumerations to the side for the time being and concentrate on other improvements for the typings?

[hpinkos]

@bampakoa sounds good to me! We'll absolutely take incremental improvements.

[bampakoa]

@hpinkos I have pushed changes regarding Core, DataSources and Renderer folders. There are some differences though after running the documentation locally, particularly in the Renderer domain:

All following now appear in the Documentation which seems reasonable to me since they are not documented as private in the jsdoc. What do you think?

• CubeMap
• Texture
• UniformFloat
• UniformFloatVec2
• UniformFloatVec3
• UniformFloatVec4
• UniformSampler
• UniformInt
• UniformIntVec2
• UniformIntVec3
• UniformIntVec4
• UniformMat2
• UniformMat3
• UniformMat4
• UniformArrayFloat
• UniformArrayFloatVec2
• UniformArrayFloatVec3
• UniformArrayFloatVec4
• UniformArraySampler
• UniformArrayInt
• UniformArrayIntVec2
• UniformArrayIntVec3
• UniformArrayIntVec4
• UniformArrayMat2
• UniformArrayMat3
• UniformArrayMat4

[hpinkos]

Thanks @bampakoa! I'll take a look at these changes soon

[cesium-concierge]

Thanks again for your contribution @bampakoa!

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.

---

I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome.

🌍 🌎 🌏

[hpinkos]

@bampakoa sorry for the long wait on this. I think after those two comments are addressed this will be ready to go

[bampakoa]

here it is @hpinkos . Sorry about the missing things. Can you please check and if everything is fine, I could go on with the rest of the PR (have a look in the checklist)

[hpinkos]

Thanks @bampakoa! I just pushed up a few changes

[bampakoa]

@hpinkos shall I continue with a separate PR for the remaining items since you have merged and closed this one already?

Also, is it better for you to have one PR per folder for Scene, Widgets and Workers?

[hpinkos]

@bampakoa separate smaller PRs are easier to get merged. I think it'd be fine if you did one for scene and one for widgets/workers though