Update JSDoc comments

[wipfli]

Documentation of function signatures includes the types of the arguments. These type annotations are redundant in TypeScript and are not used by documentation.js, which we use to produce the docs website. I suggest we remove all JSDocs type annotations.

For example:

    /**
     * Sets or clears the map's maximum pitch.
     * If the map's current pitch is higher than the new maximum,
     * the map will pitch to the new maximum.
     *
     * @param {number | null | undefined} maxPitch The maximum pitch to set (0-85). Values greater than 60 degrees are experimental and may result in rendering issues. If you encounter any, please raise an issue with details in the MapLibre project.
     *   If `null` or `undefined` is provided, the function removes the current maximum pitch (sets it to 60).
     * @returns {Map} `this`
     */
    setMaxPitch(maxPitch?: number | null) {

should be

    /**
     * Sets or clears the map's maximum pitch.
     * If the map's current pitch is higher than the new maximum,
     * the map will pitch to the new maximum.
     *
     * @param maxPitch The maximum pitch to set (0-85). Values greater than 60 degrees are experimental and may result in rendering issues. If you encounter any, please raise an issue with details in the MapLibre project.
     *   If `null` or `undefined` is provided, the function removes the current maximum pitch (sets it to 60).
     * @returns `this`
     */
    setMaxPitch(maxPitch?: number | null) {

[HarelM]

Are you sure?
I've updated a jsdoc comment from object to an actual type in map cameraForBounds recently, I was wondering if this what will make the docs correct...

[HarelM]

I have no clue for example why fitBounds is not accurate in the docs... Can you explain?

[wipfli]

For fitBounds, this is the source code:

    /**
     * Pans and zooms the map to contain its visible area within the specified geographical bounds.
     * This function will also reset the map's bearing to 0 if bearing is nonzero.
     *
     * @memberof Map#
     * @param bounds Center these bounds in the viewport and use the highest
     *      zoom level up to and including `Map#getMaxZoom()` that fits them in the viewport.
     * @param {FitBoundsOptions} [options] Options supports all properties from {@link AnimationOptions} and {@link CameraOptions} in addition to the fields below.
     * @param {number | PaddingOptions} [options.padding] The amount of padding in pixels to add to the given bounds.
     * @param {boolean} [options.linear=false] If `true`, the map transitions using
     *     {@link Map#easeTo}. If `false`, the map transitions using {@link Map#flyTo}. See
     *     those functions and {@link AnimationOptions} for information about options available.
     * @param {Function} [options.easing] An easing function for the animated transition. See {@link AnimationOptions}.
     * @param {PointLike} [options.offset=[0, 0]] The center of the given bounds relative to the map's center, measured in pixels.
     * @param {number} [options.maxZoom] The maximum zoom level to allow when the map view transitions to the specified bounds.
     * @param {Object} [eventData] Additional properties to be added to event objects of events triggered by this method.
     * @fires movestart
     * @fires moveend
     * @returns {Map} `this`
	 * @example
     * var bbox = [[-79, 43], [-73, 45]];
     * map.fitBounds(bbox, {
     *   padding: {top: 10, bottom:25, left: 15, right: 5}
     * });
     * @see [Fit a map to a bounding box](https://maplibre.org/maplibre-gl-js-docs/example/fitbounds/)
     */
    fitBounds(bounds: LngLatBoundsLike, options?: FitBoundsOptions, eventData?: any) {
        return this._fitInternal(
            this.cameraForBounds(bounds, options),
            options,
            eventData);
    }

and this is the documentation website:

What is not accurate about it?

[HarelM]

IDK, it was any when I looked at it, maybe a refresh was needed.
Also it needs to have @link now that I see that it's grayed out, but that's not related.
In any case, I trust you understand this more than I do, so just let me know what's the right way to go :-)

[birkskyum]

Can we leverage the tsdoc format somehow?

[wipfli]

I wonder how to best document string literal types in TypeScript? We introduced some in the TypeScript migration.

Related issue:

microsoft/TypeScript#38106

[wipfli]

@HarelM, things that are grayed out have no documentation comments. For example, the definition of FitBoundsOptions has no a documentation comment:

maplibre-gl-js/src/ui/camera.ts

Lines 90 to 101 in 3e23fbf

	export type EaseToOptions = AnimationOptions & CameraOptions & {
	delayEndEvents?: number;
	padding?: number | RequireAtLeastOne<PaddingOptions>;
	}
	export type FitBoundsOptions = FlyToOptions & {
	linear?: boolean;
	offset?: PointLike;
	maxZoom?: number;
	maxDuration?: number;
	padding?: number | RequireAtLeastOne<PaddingOptions>;
	}

[HarelM]

Ahh, interesting, thanks!

[wipfli]

Google's TypeScript style guide also says to not document redundant types: https://google.github.io/styleguide/tsguide.html#omit-comments-that-are-redundant-with-typescript

[wipfli]

I find it very confusing that JavaScript + JSDoc basically is TypeScript...

[wipfli]

The docs website uses documentation.js to parse JSDoc comments in the MapLibre GL JS source code. After parsing, all information is written into a file called api.json. This is the responsible command in the docs repo:

https://github.com/maplibre/maplibre-gl-js-docs/blob/3fac88eb4456b2bce5b443e800e5906b9b20a97b/package.json#L71

Batfish, a Mapbox static site generator, consumes api.json and builds the docs website based in its content.

[wipfli]

GL JS might benefit in the long run if we were to migrate the JSDoc comments to the TSDoc format. Currently, we have to manually sync the types in the docs to the source code types. This is error-prone and redundant. The migration to TypeScript #209 introduced many new types which miss documentation. For example, MapOptions is the argument type of the Map class constructor. While the class has documentation, MapOptions does not.

[wipfli]

I think we should migrate from JSDoc to TSDoc comments, and replace documentation.js with TypeDoc. TypeDoc can output an api.json file, too. We would need to adapt the docs website to consume a differently organized api.json file.

[HarelM]

Sounds like a good idea.

[birkskyum]

@wipfli , it sure seems like the right thing to do long term. If this tsdoc .json file happen, I'd be happy to take a swing at adapting our docs. For maintenance reasons, I think we could also benefit of migrate from these mapbox-only tools like batfish and jsxtreme with limited maintenance and docs (ironically) to something more standardized - and it could be part of this too.

[HarelM]

+1 for not using mapbox proprietary stuff.

[wipfli]

I am leaning towards using the existing documentationjs+jsdoc system and adding documentation to types which are not documented yet. Maybe this produces more redundancy than using tsdoc, but I think we can manage it. Rebuilding the docs website on typedoc's output seems to me a much larger task than aligning the existing framework to the changes of #209.

[birkskyum]

@wipfli I agree - for now at least. I experimented with generating the .json from typedoc, but it only had a small part of the information we get now with the current setup, and alternatively we could use the tsdoc backed project https://api-extractor.com/, but it doesn't seem that easy to use yet, and it also seems to be tied to the rushstack etc. which is an entirely different story.

[HarelM]

We can leave this issue open if we decide we would like to do this later or close this if we decide that we are not planning to pursue it. Either way works for me.

[wipfli]

Let's leave it open. I would like to update the jsdocs comments. There are some typos I would like to fix. Using ts function arguement types is also something I want.

Removing parameter types for function signatures only does not seem good, because for typedefs and class constructors we would still need to put them. That would make the JSDoc style inconsistent.

I close this issue for now. I thought there would be some easy gains to make. But after looking into alternatives, it seems like the current solution is not all that bad.