Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix buildModuleUrl.setBaseUrl and .getCesiumBaseUrl in JSDoc/TypeScript #9783

Closed
wants to merge 1 commit into from
Closed

Conversation

puxiao
Copy link
Contributor

@puxiao puxiao commented Sep 1, 2021

Issue 8922 8924

Description

If we use TypeScript, we will encounter the following problems.

import { buildModuleUrl } from 'cesium'

buildModuleUrl.setBaseUrl('./static/Cesium/')
buildModuleUrl.getCesiumBaseUrl()

PROBLEMS:

Property 'setBaseUrl' does not exist on type '(relativeUrl: string) => string'.
Property 'getCesiumBaseUrl' does not exist on type '(relativeUrl: string) => string'.

Why?

Source/Core/buildModuleUrl.js

/**
 * Given a relative URL under the Cesium base URL, returns an absolute URL.
 * @function
 *
 * ...
 */
function buildModuleUrl(relativeUrl) {
  ...
}



/**
 * Sets the base URL for resolving modules.
 * @param {String} value The new base URL.
 */
buildModuleUrl.setBaseUrl = function (value) {
  ...
};

/**
 * Gets the base URL for resolving modules.
 */
buildModuleUrl.getCesiumBaseUrl = getCesiumBaseUrl;

We can see that:

  1. buildModuleUrl is a function
  2. buildModuleUrl also is a namespace

When we execute the command build-ts, this is very confusing for JSDoc.

Cesium.d.ts

/**
 * Given a relative URL under the Cesium base URL, returns an absolute URL.
 * @example
 * ...
 */
export function buildModuleUrl(relativeUrl: string): string;

We can see that buildModuleUrl.setBaseUrl and buildModuleUrl.getCesiumBaseUrl is not exported.


Wrong solution

I have tried but failed.

+ /** @namespace buildModuleUrl */

 /**
  * @function
  */
 function buildModuleUrl(relativeUrl) {
   ...
 }


 /**
+ * @memberof buildModuleUrl
  * ...
  */
 buildModuleUrl.setBaseUrl = function (value) {
   ...
 };

 /**
+ * @memberof buildModuleUrl
  * ...
  */
 buildModuleUrl.getCesiumBaseUrl = getCesiumBaseUrl;

yarn run build-ts

Cesium.d.ts

export namespace buildModuleUrl {
    /**
     * ...
     */
    function setBaseUrl(value: string): void;
    /**
     * ...
     */
    function getCesiumBaseUrl(): Resource;
}

This time buildModuleUrl.setBaseUrl and buildModuleUrl.getCesiumBaseUrl is exported.

But export function buildModuleUrl has disappeared.


Emm.... I'm tired, destroy it.


The right solution

After unremitting efforts, I finally found a solution.

https://groups.google.com/g/jsdoc-users/c/1Md5S8j2ZI4

  1. We use buildModuleUrl to represent the function
  2. We use buildModuleUrl(1) to represent the namespace

Source/Core/buildModuleUrl.js

  /**
   * Given a relative URL under the Cesium base URL, returns an absolute URL.
+  * @function buildModuleUrl
   *
   * ...
   */
  function buildModuleUrl(relativeUrl) {
    ...
  }

+ /** @namespace buildModuleUrl(1) */

  /**
+  * @function setBaseUrl
+  * @memberof buildModuleUrl(1)
   * ...
   */
  buildModuleUrl.setBaseUrl = function (value) {
    ...
  };

  /**
+  * @function getCesiumBaseUrl
+  * @memberof buildModuleUrl(1)
   * ...
   */
  buildModuleUrl.getCesiumBaseUrl = getCesiumBaseUrl;

yarn run build-ts

Wow...

/**
 * Given a relative URL under the Cesium base URL, returns an absolute URL.
 * @example
 * var viewer = new Cesium.Viewer("cesiumContainer", {
 *   imageryProvider: new Cesium.TileMapServiceImageryProvider({
 *   url: Cesium.buildModuleUrl("Assets/Textures/NaturalEarthII"),
 *   }),
 *   baseLayerPicker: false,
 * });
 * @param relativeUrl - The relative path.
 * @returns The absolutely URL representation of the provided path.
 */
export function buildModuleUrl(relativeUrl: string): string;

export namespace buildModuleUrl {
    /**
     * Sets the base URL for resolving modules.
     * @param value - The new base URL.
     * @returns void
     */
    function setBaseUrl(value: string): void;
    /**
     * Gets the base URL for resolving modules.
     * @returns The base URL for resolving modules.
     */
    function getCesiumBaseUrl(): Resource;
}

Perfect !!!

@cesium-concierge
Copy link

Thanks for the pull request @puxiao!

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

@cesium-concierge
Copy link

Thanks again for your contribution @puxiao!

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.

@puxiao
Copy link
Contributor Author

puxiao commented Dec 2, 2021

@ebogo1
Please code review this pr.

@ggetz
Copy link
Contributor

ggetz commented Jan 19, 2022

Thanks @puxiao! Unfortunately, this does not work correctly with the generated documentation (npm run generateDocumentation ). I see two buildModuleUrl entries and see the (1).

image

In other modules, we use @memberof without defining a namespace or using an explicit name for @function. Does that work in your case?

* @param {String} value The new base URL.
* @return {Void} void
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Throughout the codebase, we omit the return tag if the return type is void.

@puxiao
Copy link
Contributor Author

puxiao commented Jan 24, 2022

I've tried to use @memberof and @function, but it doesn't solve the problem.

Wrong solution

I have tried but failed.

/**

  • @function
    */
    function buildModuleUrl(relativeUrl) {
    ...
    }

/**

    • @memberof buildModuleUrl
    • ...
      */
      buildModuleUrl.setBaseUrl = function (value) {
      ...
      };

/**

    • @memberof buildModuleUrl
    • ...
      */
      buildModuleUrl.getCesiumBaseUrl = getCesiumBaseUrl;
      yarn run build-ts

Cesium.d.ts

export namespace buildModuleUrl {
/**
* ...
/
function setBaseUrl(value: string): void;
/
*
* ...
*/
function getCesiumBaseUrl(): Resource;
}
This time buildModuleUrl.setBaseUrl and buildModuleUrl.getCesiumBaseUrl is exported.

But export function buildModuleUrl has disappeared.

@ggetz
Copy link
Contributor

ggetz commented Feb 8, 2022

Thanks for taking a look @puxiao. Unfortunately, since this change breaks existing documentation, I don't think we can take it as is. Also, while this function is technically public, I don't believe we recommend it for use in your apps. What is the use case for using this function?

@ggetz ggetz self-assigned this Feb 8, 2022
@puxiao
Copy link
Contributor Author

puxiao commented Feb 8, 2022

Okay, I know.

Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Issue/PR closed
Development

Successfully merging this pull request may close these issues.

4 participants