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: typescript issues #9019

Closed
wants to merge 0 commits into from
Closed

fix: typescript issues #9019

wants to merge 0 commits into from

Conversation

jony89
Copy link
Contributor

@jony89 jony89 commented Jul 5, 2020

Changes I've had to made in order to get typescript working. all changes are tested and working. I always working with the examples in sandcastle and the documentation.

reviews, and changes requests are welcome.

@cesium-concierge
Copy link

Thanks for the pull request @jony89!

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

@hpinkos
Copy link
Contributor

hpinkos commented Jul 20, 2020

@mramato please review

@mramato
Copy link
Contributor

mramato commented Jul 20, 2020

@jony89 I appreciate the PR but we can't take any of these changes except for the Viewer.js constructor fix.

As I mentioned to you in my comment here, #8898 (comment), TypeScript doesn't support getters/setters of differing types. For example, this PR breaks the below code:

const entity = new Entity();
entity.position = new Cartesian3(1,2,3);
const currentPosition = entity.position.getValue(JulianDate.now());

Because TypeScript thinks that entity.position is a Cartesian3, but it is not. It is a PositionProperty.

If there's something we're missing that would allow us to take a Cartesian3 in position but always return a PositionProperty, that would be great, but I'm fairly certain this simply isn't possible to express in TypeScript.

@jony89
Copy link
Contributor Author

jony89 commented Jul 21, 2020

@mramato Then how should one set the position property? or the material property with Color? both inline or using the entity constructor,

PositionProperty constructor does not support any arguments

@kring
Copy link
Member

kring commented Jul 21, 2020

@jony89 if your position isn't changing with time, use ConstantPositionProperty.

@jony89
Copy link
Contributor Author

jony89 commented Jul 21, 2020

What if it does? what about PolygonHierarchy ? CallbackProperty for dynamic options, and Color values for material properties. there are many more examples. Maybe we need to rethink things here so the API will fit typescript

@mramato Anyway I changed the PR and kept the viewer change only

@kring
Copy link
Member

kring commented Jul 21, 2020

The documentation and type definition tell you which base type of property you need to use, e.g. Property or PositionProperty. Then you choose a subclass of those based to instantiate, e.g. ConstantProperty, ConstantPositionProperty, or SampledPositionProperty. If you were previously just assigning a value (not a Property type), then you should always use the Constant- version to get equivalent behavior.

MaterialProperty is the only one that is a bit different. There, you choose a subclass based on the type of material (e.g. ColorMaterialProperty) and pass one or more instances of Property subclasses to its constructor.

@mramato as an aside, I wonder if we're thinking about this wrong. Users of CesiumJS are much more likely to set entity properties than they are to get them. So maybe we should change the type definitions to correctly express that you're allowed to set with a simple primitive type. That means it will (incorrectly) think that that the getter might give you back a primitive type, which is not true, but in this less common and more advanced use-case, it's easy to work around that annoyance with a type assertion (cast). I'm not sure if I'm convinced on this, just throwing it out for consideration.

@courageon
Copy link

@jony89 Just came across the "position" issue in my code and Google brought me here. There is hacky work-around where you can cast the position property as an "any". It's real ugly though... I'm going to try and isolate that bit in my code in case of future changes :)

(entity.position as any) = newCartesian3Position;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants