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

Diverse typescript types errors #8924

Closed
Crocsx opened this issue Jun 7, 2020 · 10 comments
Closed

Diverse typescript types errors #8924

Crocsx opened this issue Jun 7, 2020 · 10 comments

Comments

@Crocsx
Copy link

Crocsx commented Jun 7, 2020

Hello,

Thanks for bringing in built in types in cesium, it's a really nice addition

I have the following thing that I believe is missing (and other but the issues are already posted).

  1. Can't import when from Cesium

like in this code :

var promise = Cesium.sampleTerrain(terrainProvider, 11, positions);
Cesium.when(promise, function(updatedPositions) {
     // positions[0].height and positions[1].height have been updated.
    // updatedPositions is just a reference to positions.
});

can't find when from cesium

  1. Polygon can't be used at geometry

I can't do this anymore, which I believe should work from the docs (https://cesium.com/docs/cesiumjs-ref-doc/GeometryInstance.html)

      const rectangleInstance = new GeometryInstance({
        geometry: new PolygonGeometry({
          polygonHierarchy: new PolygonHierarchy(AreaUtils.getAreaCoordinates(area)),
        }),
      });

Type 'PolygonGeometry' is missing the following properties from type 'Geometry': attributes, indices, primitiveType, boundingSphere

  1. Can't add IonImageryProvider to Imagerylayer

const newLayer = new ImageryLayer(new IonImageryProvider({ assetId: provider.id }));

Argument of type 'IonImageryProvider' is not assignable to parameter of type 'ImageryProvider'.
Type 'IonImageryProvider' is missing the following properties from type 'ImageryProvider': defaultNightAlpha, defaultDayAlphats(2345)

@Crocsx Crocsx changed the title Can't import when from Cesium Diverse typescript types errors Jun 7, 2020
@mramato
Copy link
Contributor

mramato commented Jun 7, 2020

Thanks for taking the time to write this up @Crocsx! Your feedback is really appreciated.

  1. Can't import when from Cesium

when has never been part of the official Cesium API and we discourage users from using it in their own applications. (it also doesn't appear in our docs). For your own code example is there a particular reason you don't just call then directly?

Cesium.sampleTerrain(terrainProvider, 11, positions)
.then(function(updatedPositions) {
     // positions[0].height and positions[1].height have been updated.
    // updatedPositions is just a reference to positions.
}));

Since technically, when is a little nonconformat for Promises, the absolute best practice for code like this in a TS application would be to wrap it in a resolve:

Promise.resolve(Cesium.sampleTerrain(terrainProvider, 11, positions))
.then(function(updatedPositions) {
     // positions[0].height and positions[1].height have been updated.
    // updatedPositions is just a reference to positions.
}));

I left off the catch for simplicity, but that's a good idea as well. If there is specific reason you still feel you still need access to when, please let us know why, but Promise.resolve or new Promise are the best and most technically correct approach for TS applications.

  1. Polygon can't be used at geometry

Good catch, we just added the ability to create some smokescreen tests for TypeScript and I think all of the geometries are an excellent candidate for testing here. We'll fix this and add some tests to make sure they maintain the correct interface in TS going forward.

  1. Can't add IonImageryProvider to Imagerylayer

Happy to say that this one is already fixed by #8908 in master.

I'll let you know once the PR for number 2 is open and we should be able to get it into a 1.70.1 patch release we're doing this week specifically for TypeScript bugfixes and improvements.

Thanks again!

@mramato
Copy link
Contributor

mramato commented Jun 7, 2020

Started looking into 2 and it's a bit more nuanced than I thought. Basically in some cases we take both the geometry type, i.e. BoxGeometry and Geometry instances that are created by it. i.e. BoxGeometry is not actually a geometry but has a function called BoxGeometry.createGeometry which we call to create the actual Geometry instance.

We don't clearly define this anywhere and it's definitely confusing from an API naming situation. The best solution is most likely to define a new type out of whole cloth called GeometryFactory or something and it would have a single static function called createGeometry. So all of our XXXGeometry functions would actually be GeometryFactory and the regular Geometry remains unchanged.

@Crocsx
Copy link
Author

Crocsx commented Jun 7, 2020

@mramato thank for the reply.
I always used cesium when, cause I've seen it in some example like (https://cesium.com/docs/cesiumjs-ref-doc/sampleTerrain.html) and I thought it was the way to go. I will change my code then thanks.

Thanks you for the update and the future 1.70.1.

@kring
Copy link
Member

kring commented Jun 8, 2020

@mramato I think we should just expose when. Using Promise.resolve will mean using two different promise implementations (especially if the user needs to polyfill it for IE11 support or whatever). That makes some sense for apps that make extensive use of promises, but not for apps that are just using them for Cesium. And just using .then requires the user to at least check for undefined, and I think there are still some cases of functions in Cesium (even though it's poor practice) that may either return a promise or a value directly. If when is available, it can be used to paper over that difference, as in the sampleTerrain example above.

It means we'll have to deprecate it later, but switching out the promise implementation is a big breaking change anyway, if for no other reason then because calls to .otherwise in user code will have to change to .catch. Exposing when will actually help call attention to the problem later when we remove it.

@mramato
Copy link
Contributor

mramato commented Jun 9, 2020

And just using .then requires the user to at least check for undefined, and I think there are still some cases of functions in Cesium (even though it's poor practice) that may either return a promise or a value directly. If when is available, it can be used to paper over that difference, as in the sampleTerrain example above.

What I don't understand is how is using when different than Promise.resolve in the situations you're suggesting? They would be all but functionally identical with the added bonus of always having a spec-conformant promise.

@kring
Copy link
Member

kring commented Jun 10, 2020

Promise.resolve isn't available on IE11 without a polyfill. Every Cesium user already has when.

@Crocsx
Copy link
Author

Crocsx commented Aug 23, 2020

@mramato I would like to add that

in this issues => #8922

export namespace buildModuleUrl {
    function setBaseUrl(value: string): void;
}

as been modified to

export namespace buildModuleUrl {
    function setBaseUrl(value: string): string;
}

but I believe that since 1.10 there is a method (that I use in my angular app)

Added buildModuleUrl.setBaseUrl function to allow the Cesium base URL to be set without the use of the global CESIUM_BASE_URL variable.

at the moment this return an error buildModuleUrl.setBaseUrl('/assets/cesium/');

Property 'setBaseUrl' does not exist on type '{ (relativeUrl: string): string; (relativeUrl: string): string; (relativeUrl: string): string; }'

@lovewinders
Copy link

@mramato I would like to add that

in this issues => #8922

export namespace buildModuleUrl {
    function setBaseUrl(value: string): void;
}

as been modified to

export namespace buildModuleUrl {
    function setBaseUrl(value: string): string;
}

but I believe that since 1.10 there is a method (that I use in my angular app)

Added buildModuleUrl.setBaseUrl function to allow the Cesium base URL to be set without the use of the global CESIUM_BASE_URL variable.

at the moment this return an error buildModuleUrl.setBaseUrl('/assets/cesium/');

Property 'setBaseUrl' does not exist on type '{ (relativeUrl: string): string; (relativeUrl: string): string; (relativeUrl: string): string; }'

I have the same problem

@thw0rted
Copy link
Contributor

Trackback to #9297, more interactions between TS and when not-quite-Promises.

@ggetz
Copy link
Contributor

ggetz commented Apr 5, 2022

After #9297 and #8922, I believe there's nothing actionable left in this issue. Closing, but feel free to open any new issues if you see typescript errors.

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

No branches or pull requests

7 participants