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

buildModuleUrl not exported in TypeScript #8922

Open
jeanpul opened this issue Jun 5, 2020 · 4 comments · Fixed by #8923
Open

buildModuleUrl not exported in TypeScript #8922

jeanpul opened this issue Jun 5, 2020 · 4 comments · Fixed by #8923

Comments

@jeanpul
Copy link

jeanpul commented Jun 5, 2020

Hello,

From Cesium 1.7

import { buildModuleUrl } from 'cesium';
leads to Module '"cesium"' has no exported member 'buildModuleUrl'

to fix it I've changed Cesium.d.ts :

// in the declare module "cesium" part

export function buildModuleUrl(value: string);

export namespace buildModuleUrl {

function setBaseUrl(value: string): void;

}

// after the declare module "cesium" part

declare module "cesium/Source/Core/buildModuleUrl" { import { buildModuleUrl } from 'cesium'; export default buildModuleUrl; }

thx

@mramato
Copy link
Contributor

mramato commented Jun 5, 2020

Thanks for bringing this up @jeanpul!

Oddly enough, buildModuleUrl was never part of the public API and has always been a private function. This is now obvious because TypeScript only includes the officially supported API.

That being said, the fact that it is not part of the official API is indeed a bug. We even use it in Sandcastle examples. I'll open a PR to add JSDoc for it and this fix will go out early next week for a 1.70.1 patch release that we want to put out specifically to improve these new TS definitions.

Thanks again.

mramato added a commit that referenced this issue Jun 5, 2020
It's been used in Sandcastle examples forever, but has always been marked
as private in jsdoc. In order to make it available in TypeScript, we need
to expose it.

Also cleaned up the doc to be more inline with what the current version
actually does.

Fixes #8922
@jeanpul
Copy link
Author

jeanpul commented Jun 16, 2020

Hello,

I've checked the 1.70.1 release and in the Cesium.d.ts still have a problem with buildModuleUrl :
The generated signature is
export function buildModuleUrl(relativeUrl: string): void;
but should be
export function buildModuleUrl(relativeUrl: string): string;
in order to returns string

Thx

@mramato
Copy link
Contributor

mramato commented Jun 16, 2020

Thanks @jeanpul I pushed 638c6eb directly to master with the fix and it will go out with our next release.

katSchmid added a commit to PropellerAero/cesium that referenced this issue Jun 24, 2020
* Add translucency rectangle to sandcastle example [skip ci]

* Account for primitive restart value.

* No const in ES5.

* Simplify skinning to always expect VEC4 joint/weight accessors.

* Changed code mirror viewport margin to Infinity for improved search

* Moves changes to 1.70

* Remove unused imports.

* Fix variable and semantic names.

* Added frustumSplits option to DebugCameraPrimitive

* updated CHANGES.md

* Added GlobeTranslucencyState

* Fixed ground primitives in orthographic mode

* Changed czm_writeDepthClampedToFarPlane to czm_writeDepthClamp and czm_depthClampFarPlane to czm_depthClamp

* Add cjs extension to prettier configuration

I was modifying gulpfile in another branch and realized formatting was
not set up correctly.

* Fixed depth plane for orthographic

* updated CHANGES.md

* Made orthographic depth plane aligned to camera and fixed multifrustum ray ellipsoid intersection

* made common variable for camera

* Fixed derived commands

* Remove invisible translucency state to support classification on invisible surfaces

* Fix picking in 2D

* Use better image for sandcastle [skip ci]

* Don't do depth only shader if clipping planes are enabled [skip ci]

* Updated unit tests

* Fix a typo

* Fix a typo

* Render full sky atmosphere when globe is hidden

* Add per-fragment atmosphere, improve per-vertex atmosphere

* Update CHANGES.md

* Fix tests

* Add underground color

* Add getters/setters, Sandcastle, tests

* Update CHANGES.md

* Fix CHANGES.md [skip ci]

* Added nightAlpha and dayAlpha in the ImageryLayer

* Updated CONTRIBUTORS.md

* CHANGES.md updated

* Added night textures showcase to the Sandcastle

* typo

* Switch to two-pass classification

* Updates from review

* Tweak atmosphere under ellipsoid so it's less bright

* Fix tests

* removed old comment [skip ci]

* Distance relative to globe surface

* Rename undergroundColorByDistance to undergroundColorAlphaByDistance

* Updates from review

* Doc [skip ci]

* Render underground when clipping planes or cartographic limit are being used

* Add czm_backFacing

* Only render ground atmosphere for front faces

* Show back faces in cartogrphic limit rectangle sandcastle

* Fix camera height uniform

* Temp

* Align ellipsoid geometry to camera

* Default perFragmentAtmosphere to false

* Support #rgba and #rrggbbaa in Color.fromCssColorString

* Updates from review

* Same blueness everywhere

* Improve alpha

* Tweak brightness by fade distance

* Revert ground atmosphere change to fix issues when zoomed out really far

* Updates from review

* Remove old intersection code

* Fix lighting crash

* Tweak strength to prevent ring artifact in enableLighting mode

* Turn off globe lighting in demo

* Reorder ifdef's

* Reorder updateFrameState

* Added GlobeTranslucency to group translucency options

* Fix test

* Remove frameState from update function

* Premultiply alpha for ground polylines

* Fix tests

* Fix translucencyEnabled not applying when base color alpha is < 1.0

* Define constant once

* Add back translucency gradient to sky atmosphere

* Add define for globe translucency to SkyAtmosphereCommon

* Sky atmosphere fix to not use camera height when intersecting the ellipsoid

* some tweaks to day/night blending

* Generate official TypeScript type definitions

It's been a long requested feature for us to have official TypeScript type
definitions.  While the community has done a yeoman's job of manually
supporting various efforts, the most recent incarnation of which is
`@types/cesium`, the sheer scale and ever-evolving nature of Cesium's code
base makes manual maintenance a Sisyphean task.

Thankfully, our decision to maintain meticulous JSDoc API documentation
continues to pay dividends and is what makes automatically generating
TypeScript definitions possible. Using the excellent
https://github.com/englercj/tsd-jsdoc project we can now automatically
generate and even partially validate official definitions as part of the
build process. (Thanks to @bampakoa who contributed some early PRs to both
CesiumJS and tsd-jsdoc over a year ago and is how I learned about
tsd-jsdoc)

While tsd-jsdoc output is mostly where we need it to be, we do
post-processing on it as well. This lets us clean up the output and also
make sure these definitions work whether users include cesium via module,
i.e. `import { Cartesian3 } from 'cesium'`, or individual files, i.e.
`'import Cartesian3 from 'cesium/Source/Core/Cartesian3'`. There were also
some quirks of tsd-jsdoc output we fixed that may eventually turn into a PR
into that project from us. The post-processing is part typescript compiler
API, part string manipulation. It works and is straightforward but we might
want to go full TS api in the future if we decide we need to do more
complicated tasks. The output of tsd-jsdoc is currently a little noisy
because of some incorrect error reporting, but I'm talking with the
maintainer in englercj/tsd-jsdoc#133 to get them
fixed. No need to hold up this PR for it though.

The definition is generated as a single `Cesium.d.ts` file in Source, so it
lives alongside Cesium.js. It is ignored by git but generated by a separate
`build-ts` task as part of CI and makeZipFile. This task also validates the
file by compiling it with TypeScript, so if a developer does anything too
egregious, the build will fail. Definitions are automatically included in
our npm packages and release zips and will be automatically used by IDEs
thanks to the `types` setting in package.json. This means that IDEs such as
VS Code will prefer these types over the existing `@types/cesium` version
by default.

I didn't want to slow the `build` step down, so I made this a separate
step, but in the future we could consider running it by default and we
could also unignore this file in Git so that PR reviewers can see the
impact, if any, our code changes have on the generated definitions. This
might be a good idea as an additional sanity check and should only actually
change when the public API itself changes. But the issue would be
remembering to run it before submitting the code (or we could use git hooks
I suppose?) I just don't want to slow down devs so I'm hesitant to do
anything like this out of the gate. We can definitely revisit in the
future.

A particular exciting thing about this approach is that it exposed a ton of
badness in our current JSDoc markup, which is now fixed. Previously, our
only concern was "does the doc look right" and we didn't pay attention to
whether the meta information generated by JSDoc correctly captured type
information (because up until it didn't matter). We leaned particular hard
on `@exports` which acted as a catch-all but has now been completely
removed from the codebase. All this means is that our documentation as a
whole has now improved greatly and will continue to be maintained at this
new higher level thanks to incorporating TS definition creation into our
pipeline!

One minor caveat here is that obviously we changed our JSDoc usage to both
make it correct and also accommodate TypeScript. The main drawback to these
fixes is that enums are now generated as globals in the doc, rather than
modules. This means they no longer have their own dedicated page and are
instead on the globals page, but I changed the code to ensure they are
still in the table of contents that we generate. I think this trade-off is
perfectly fine, but I wanted to mention it since it does change the doc
some. We can certainly look into whether we can generate enums on their own
page if we think that makes sense. (I actually like this approach a little
better personally).

Last major piece, the actual code. 99% of the changes in this PR only
affect the JSDoc. There are two exceptions:

A few of our enums also have private functions tacked onto them. I had to
move these functions to be outside the initializer but otherwise they are
unchanged.  This ensures that a valid TS enum is generated from our code, since you can't have functions globbed onto enums in the TS world. If we were writing TS code by hand, we could use  declaration merging with a namespace, but the toolchain we are using doesn't have a way to express that right now.  There were two cases where these extra functions weren't private, `ComponentDataType` and `IndexDataType`. That means that as far as the TS definitions goes, the helper methods don't exist.  I consder this an edge case and we can write up issues to investigate later.  I'm actually not even sure if these functions are public on purposes, @lilleyse can you confirm?

We had a few places where we had method signatures with optional parameters
that came _before_ required parameters, which is silly. This is invalid
TypeScript (and not good API design no matter the language). In 99% of
cases this was `equalsEpsilon` style functions where the lhs/rhs were
optional but the epsilon was not. I remember the discussion around this
when we first did it because we were paranoid about defaulting to 0, but
it's an edge case and it's silly so I just changed the epsilon functions
to default to zero now, problem solved.

Here's a high level summary of the JS changes:

* Use proper `@enum` notation instead of `@exports` for enums.

* Use proper `@namespace` instead of `@exports` for static classes.

* Use proper `@function` instead of `@exports` for standalone functions.

* Fix `Promise` markup to actually include the type in all cases, i.e.
`Promise` => `Promise<void>` or `Promise<Cartesian3[]>`.

* Fix bad markup that referenced types that do not exist (or no longer
exist) at the spec level, `Image` => `HTMLImageElement`,
`Canvas` => `HTMLCanvasElement`, etc.. `TypedArray` in particular does not
exist and much be expressed as a lsit of all applicable types,
`Int8Array|Uint8Array|Int16Array|Uint16Array...`.

* Use dot notation instead of tilde in callbacks, to better support
TypeScript, i.e. `EasingFunction~Callback` becomes
`EasingFunction.Callback`. The only exception is when we had a callback
type that was i.e. `exportKml~ModelCallback` becomes
`exportKmlModelCallback` (a module global type rather than a type on
exportKml). This is because it's not possible to have exportKml be both a
function and a namespace in this current toolchain.  Not a big deal either
way since these are meta-types used for defining callbacks but I wanted to
mention it.

* There were some edge cases where private types that were referenced in
the public API but don't exist in the JSDoc. These were trivial to fix by
either tweaking the JSDoc to avoid leaking the type or in some cases, just
as `PixelDataType`, simply exposing the private type as public.  I also
found a few cases where things were accidentally public, I marked these as
private (these were extreme edge cases so I'm not concerned about breaking
changes). Appearances took an optional `RenderState` in their options, I
just changed the type to `Object` which we can clean up further later if
we need to.

* Lots of other little misc JSDoc issues that became obvious once we
started to generate definitions (duplicate parameters for example).

Thanks again to the community for helping generate ideas and discussions
around TS definitions over the last few years and a big thanks to @javagl
for helping behind the scenes on this specific effort by evaluating a few
different approaches and workaround before we settled on this one (I'm
working on a blog with all of the gory details for those interested).

Finally, while I'm thrilled with how this turned out (all ~41000 lines
and 1.9MB of it), I can guarantee we will uncover some issues with the
type definitions as more people use it. The good news is that improving it
is now just a matter of fixing the JSDoc, which will benefit the community
as a whole and not just TS users.


Fixes CesiumGS#2730
Fixes CesiumGS#5717

* Don't hardcode ./node_modules/tsd-jsdoc

It may be different in some environments. Instead, just use the normal module name, which will be resolved according to node's module resolution rules.

* Use tsconfig.json to avoid errors in some environments.

* Add new Sandcastle example.

* Add back a "Proxy" type.

Since master documented a `Proxy` base class, create it rather than using
`DefaultProxy` everywhere.

* Add missing `update` function to all DataSource implementations

Allows them to be assignable to `DataSource` in the TS definitions.

* Minor tweak to doc and Sandcastle

* JSDoc fixes to GregorianDate and TimeIntervalCollection

* Fix JSDoc for exportKml

* Additional JSDoc/TypeScript fixes.

* Make IonImageryProvider actually implement all ImageryProvider properties

* More TypeScript fixes

1. Make Matrix 2/3/4 ArrayLike objects

2. Make PropertyBag include a string indexor so that it is a dictionary
like object. We do this by making up a new private interface and defining
it only in the definition file. JSDoc ignores it in the HTML output.

* Update CHANGES

Fix bad commit.

* Number[] -> number[] in TS definition

Add workaround for englercj/tsd-jsdoc#117

* Fix Event.raiseEvent TS generation.

* Add missing availability property to TerrainProvider classes.

* Whoops

* More JSDoc fixes and TS improvements

1. Resource has incorrect return types.
2. Improve formatting of TS definition file.

* Don't abuse EllipsoidTerrainProvider.

* Lots of small changes to improve the typescript definitions.

* Give up on trying JSDoc tags for PropertyBag

* Example of exporting ConstructorOptions interface

* Break out constructor options: Entity and Graphics

* Various small type fixes

* Selected/tracked entity can be undefined

* First pass at generic Events

* Constructor interfaces for ImageryProviders

* Load-method option interfaces for some DataSources

* Options interfaces for Viewer, Resource

* CesiumGS#8843 Clear Request cancelFunction and deferred references to free up unintentional retained request data memory

* CesiumGS#8843 Add comments

* CesiumGS#8843 Update comment

* Add CONTRIBUTORS entry

* Use function overloading for exportKml

* Update CHANGES.md

* Update CesiumSandcastle.js

* Spell out docs for Resource.QueryParameter

* Undo some changes

* Add ConstructorOptions to the rest of the ImageryProviders

* Workaround CesiumMath -> Math documentation issue

* Fix doc links that include a hash.

* Improve camera controls when underground

* Update CHANGES, tweak doc, and add test.

* Fix UrlTemplateImageryProvider

Also opt-in tsd-jsdoc so we can use @template in the future.

* More fixes found by Kevin.

* Tweak CHANGES related to TypeScript

* Remove hardcoded node_modules in gulpfile.

1. Use `npx` to execute binaries rather than hardcoded `node_modules/.bin`,
which may not always be the right location.

2. Streamline generateDocumentation by calling it sync.

* Update CHANGES to make underground visualization more prominent

* Fix doc using @member instead of @memberof

* Removed duplicate CHANGES.md line

* Add Cesium OSM Buildings note in CHANGES

* Add Sandcastle link [ci skip]

* Fix typo [ci skip]

* fixed flipy issue

* updated CHANGES.md

* added myself to CONTRIBUTORS.md

* added tests for flipY function

* Fixed Bing Maps Road option in sandcastle example

* removed extra whitespace line

* fix gulp build error

* Added Navagis to CONTRIBUTORS.md

* updated tokens, version, and changes.md

* Fix variable naming in fog sandcastle

* Add toString method for Resource class

* Tests for Resource#toString

* Use explicit cast

* Improve JSDoc/TypeScript support for Material properties

All of the MaterialProperty constructors take primitive types in addition
to Property instances (and automatically wrap the primitive type in a
`ConstantProperty`).  The JSDoc was incorrect on this leading to sub-par
TS type definitions. Also fixed a missing optional result parameter in
the JSDoc for `EllipsoidGeodesic.interpolateUsingSurfaceDistance`.

Fixes CesiumGS#8898

* Update CHANGES

Add another missing optional result fix.

* Update CHANGES for cancelled request memory leak fix

* Add additional smokescreen to build-ts

1. Add a Specs/TypeScript directory with a minimal TS configuration that
uses the d.ts files generated by build-ts

2. Have `build-ts` compile index.ts from this directory which makes sure
various types actually conform to their expected interfaces.

3. Fix ImageryProvider interfaces which had issues exposed by this new
test.

In the future we can add any additional smokescreens that we think are
important to validate our definitions going forward, but this will never
be a fully exhaustive check.

We currently don't actually execute the output, it's just there for
compile-time checking.  We can revisit this if we ever feel that it's
needed.

* Update CHANGES

* Add missing property typs to TypeScript smokescreen

* Other minor TS improvements.

* Make sure all nullable Entity API properties are marked as such.

* Most ImageryProvider properties can be undefined.

* Fix missing HeadingPitchRoll/HeadingPitchRange JSDoc

* Missing `|undefined` in GoogleEarthEnterpriseMapsProvider

* Missing JSDoc

* change cesiumScriptRegex value

fixes Issue CesiumGS#8902

* Expose buildModuleUrl as part of the Cesium API

It's been used in Sandcastle examples forever, but has always been marked
as private in jsdoc. In order to make it available in TypeScript, we need
to expose it.

Also cleaned up the doc to be more inline with what the current version
actually does.

Fixes CesiumGS#8922

* add more test

add CesiumSomething test

* Correcting Navagis Corporate Link

* Two small JSDoc/TS fixes

1. `EllipsoidTangentPlane.fromPoints` takes an array.
2. `EntityCollection.getById` and `CompositeEntityCollection.getById` can
both return undefined.

* Fix whitespace

* Update CHANGES.

Also fix missing links from previous PRs

* Add some missing readonly documentation tags

* Add false as a possible value for skyBox and SkyAtmosphere

* CesiumGS#8927: Exposing Transforms.rotationMatrixFromPositionVelocity; Adding tests for Transforms.rotationMatrixFromPositionVelocity

* CesiumGS#8927: Adding message for exposing Transforms.rotationMatrixFromPositionVelocity to CHANGES.md

* CesiumGS#8927: Running prettier on new rotationMatrixFromPositionVelocity tests

* Only include Source folder in TypeScript smokescreen tests.

We were including all Cesium directories in the TypeScript smokescreen
configuration. This meant that multiple Cesium.d.ts files were included
if you did a full build and then ran `build-ts` afterwards. This updates
the config used by the smokescreen to only include Source.

* Improve base url regex, add additional test.

* CesiumGS#8927: Simplifed rotationMatrixFromPositionVelocity tests

* Fix geometry creation in TypeScript

None of the `XXXGeometry` classes are actually `Geometry` instances. They
are instead utility classes that create geometries via their
`createGeometry` implementation. `GeometryInstance` can take either "type"
but since JS doesn't have types we never really defined what the "utility"
type is, so the TypeScript definition for `GeometryInstance` specifies that
currently only specifies `Geometry`. This means that this valid JS code
is a compiler error in TypeScript

```
const geometryInstance = new GeometryInstance({
  geometry: new PolylineGeometry({
    positions: [],
  }),
});
```

To fix this, I introduced a `GeometryFactory` base class like we have
elsewhere in the code and changed `GeometryInstance` to take either type.
This is the only place where we actually base "non-geometry Geometry" in
the API.

Happy to consider other names, like `GeometryCreator` if we don't like
factory for some reason, but I want to get this in sooner rather than
later for 1.70.1 fixes.

Also fixed an issue with tsconfig.json I introduced in my last change
which was failing to actually catch TS compile errors because it wasn't
including the Cesium.d.ts.

* Update doc/CHANGES

* Bump version and cleanup CHANGES for 1.70.1 release.

* adding release 1.70.1

* add codeship

* make tests runable

* adding propeller code changes back

* more changes form diff

* Correcting ChANGES.md and index.d.ts

Co-authored-by: Matthew Amato <[email protected]>
Co-authored-by: Sean Lilley <[email protected]>
Co-authored-by: Kevin Ring <[email protected]>
Co-authored-by: Ed Mackey <[email protected]>
Co-authored-by: Ian Lilley <[email protected]>
Co-authored-by: Sam Suhag <[email protected]>
Co-authored-by: Sanjeet Suhag <[email protected]>
Co-authored-by: Jakub Vrána <[email protected]>
Co-authored-by: Edvinas Pranka <[email protected]>
Co-authored-by: Omar Shehata <[email protected]>
Co-authored-by: Omar Shehata <[email protected]>
Co-authored-by: James Bromwell <[email protected]>
Co-authored-by: Brandon Nguyen <[email protected]>
Co-authored-by: Hannah <[email protected]>
Co-authored-by: Eli Bogomolny <[email protected]>
Co-authored-by: Peter Gagliardi <[email protected]>
Co-authored-by: Jonathan Nogueira <[email protected]>
Co-authored-by: xiaobaogeit <[email protected]>
Co-authored-by: Jonathan Nogueira <[email protected]>
Co-authored-by: Frederic Junod <[email protected]>
Co-authored-by: John Remsberg <[email protected]>
@ggetz
Copy link
Contributor

ggetz commented Apr 14, 2022

Re-opening this as there issues with the current docs for generating TS definitions for setBaseUrl and getCesiumBaseUrl. See #9783 and #10292 (comment)

@ggetz ggetz reopened this Apr 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants