Fix geometry creation in TypeScript #8941
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
|
Thanks for the pull request @mramato!
Reviewers, don't forget to make sure that:
|
|
Fixes #8924 |
|
Not sure why travis thinks those builds are pending, it's pretty clear if you click on Details that they completed successfuly. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
None of the
XXXGeometryclasses are actuallyGeometryinstances. They are instead utility classes that create geometries via theircreateGeometryimplementation.GeometryInstancecan take either "type" but since JS doesn't have types we never really defined what the "utility" type is, so the TypeScript definition forGeometryInstancespecifies that currently only specifiesGeometry. This means that this valid JS code is a compiler error in TypeScriptTo fix this, I introduced a
GeometryFactorybase class like we have elsewhere in the code and changedGeometryInstanceto take either type. This is the only place where we actually base "non-geometry Geometry" in the API.Happy to consider other names, like
GeometryCreatorif 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.