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

Add typescript definitions #183

Open
wants to merge 18 commits into
base: master
Choose a base branch
from
Open

Add typescript definitions #183

wants to merge 18 commits into from

Conversation

twelch
Copy link

@twelch twelch commented Jun 27, 2021

Type of PR

  • Feature

What is the Goal of the PR?

Add Typescript type definitions for the top-level geoblaze module. Builds on existing geojson types in DefinitelyTyped. Include them with the published NPM package.

Goal is to meet strict type definitions, no use of any

Checklist:

  • Add build step to copy the declaration file into the dist folder for distribution. Webpack may be the tool to do it.
  • Switch types in package.json to point to dist/geoblaze.d.ts
  • Migrate the GeoRaster class definition over to a declaration file in the georaster package. Could be done as a follow-on for incremental progress.
  • Ensure both default and named exports work
  • Review methods with fine toothed comb

Note that I'm uncovering some JSDoc inaccuracies/inconsistencies in the process. I'll create a separate PR for them.

package.json Outdated Show resolved Hide resolved
Comment on lines 51 to 54
export function identify(
raster: GeoRaster,
geom?: string | InputPoint | null
): number[];
Copy link
Author

Choose a reason for hiding this comment

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

My read is that while the underlying raster may be represented with smaller byte data types, DataView methods are used to extract and return values, which are of number type, so that is what this function will return. Feedback welcome -- https://microsoft.github.io/PowerBI-JavaScript/interfaces/_node_modules_typedoc_node_modules_typescript_lib_lib_es5_d_.dataview.html#getuint8

Copy link
Author

@twelch twelch Jul 15, 2021

Choose a reason for hiding this comment

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

Actually I think the type of the pixel values returned can vary based on the raster. For example can be Uint8Array. I'm not positive that is true for this identify method, but getValues() does for the Georaster. I propose a solution over in my georaster PR, and suggest what is here will work for now. I can research and look to improve -- https://github.com/GeoTIFF/georaster/pull/59/files#r670681723

src/geoblaze.d.ts Outdated Show resolved Hide resolved
@twelch twelch marked this pull request as ready for review June 27, 2021 17:37
src/geoblaze.d.ts Outdated Show resolved Hide resolved
@twelch
Copy link
Author

twelch commented Jun 27, 2021

@DanielJDufour I think this is ready for (initial) review when you have time.

I've tested with maybe 1/3 of the methods using just a single float32 raster. I'm curious if my assumption that cell values read from the raster are always number is a good one or the type might vary with the raster cell size.

To test, I simply npm linked my local geoblaze to a typescript project I already have. Something more thorough would be to add a Typescript file to the repo that runs some tests.

src/geoblaze.d.ts Outdated Show resolved Hide resolved
@twelch twelch marked this pull request as draft June 27, 2021 22:52
@twelch
Copy link
Author

twelch commented Jun 27, 2021

Back to WIP. Finding the right structure for it to import the upstream georaster types

@twelch twelch marked this pull request as ready for review June 28, 2021 00:32
@twelch
Copy link
Author

twelch commented Jul 15, 2021

As I've used more methods I've pushed some fixes. Feeling pretty tight.

Comment on lines +28 to +29
type InputPolygon = number[][][] | Polygon | Feature<Polygon>;
type InputPoint = number[] | Point | Feature<Point>;
Copy link
Author

Choose a reason for hiding this comment

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

Looks like FeatureCollection is also supported according to https://github.com/GeoTIFF/geoblaze/blob/master/src/utils/utils.module.js#L236. Not documented clearly.

Copy link
Member

Choose a reason for hiding this comment

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

Good catch!

Copy link
Member

@DanielJDufour DanielJDufour left a comment

Choose a reason for hiding this comment

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

A very impressive amount of work grappling with the nuances of Typescript and GeoBlaze! Just a left a minor nit-picky comment about spelling of GeoRaster and GeoBlaze. Other than that, I think we would be good to merge into master and publish a "prerelease" or "release candidate" to NPMJS. I could add GeoBlaze to a demo Angular app I made for GeoRasterLayer, so we could add that to the testing you are already doing. I think I could also ask some people I know who use Angular if they can try it out before we do the official next big release!!!

Thank you!!!!!!!

@@ -0,0 +1,108 @@
import { Point, Polygon, Feature, BBox } from "geojson";
import { Georaster } from "georaster";
Copy link
Member

Choose a reason for hiding this comment

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

I know it's super picky (and annoying), but could we use GeoRaster instead of Georaster? We've used Georaster before, but I'd like to migrate over to the new spelling. Sorry!

// re-rexport for easy access by downstream user
export { Georaster } from "georaster"

type GeoblazeBBox = {
Copy link
Member

Choose a reason for hiding this comment

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

Similar to above, could we use GeoBlaze... instead of Geoblaze? I know I've done a bad job of standardizing spelling, but would like to use GeoBlaze moving forward. Sorry again!

Comment on lines +28 to +29
type InputPolygon = number[][][] | Polygon | Feature<Polygon>;
type InputPoint = number[] | Point | Feature<Point>;
Copy link
Member

Choose a reason for hiding this comment

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

Good catch!

@mjernil
Copy link

mjernil commented Jun 15, 2023

Hello, Is this PR active? I'm using typescript and this would be very useful for me

@lamabiker
Copy link

Hello, my team would also greatly benefit from having these TS defs. I can also help out if need be. Thanks!

@acao
Copy link

acao commented Jun 22, 2023

I finished up these type definitions at my last job and had intended to contribute to them, but there were layoffs, I will ask my former manager if he can contribute them, it only took a few hours!

@lamabiker
Copy link

lamabiker commented Jun 23, 2023 via email

@DanielJDufour
Copy link
Member

Thank you!

@StevenLangbroek
Copy link

I finished up these type definitions at my last job and had intended to contribute to them, but there were layoffs, I will ask my former manager if he can contribute them, it only took a few hours!

Here you go: https://gist.github.com/StevenLangbroek/3de4e7e67cf49c764edf449da319bf69

I had to make a small tweak for our use-case; I named the histogram key of the BlazeStats type, but since there was already a Histogram interface I gave it the clunky BlazeHistogram name. Not ideal, but I'm sure y'all can come up with a better name ✌️

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