-
Notifications
You must be signed in to change notification settings - Fork 28
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
base: master
Are you sure you want to change the base?
Conversation
src/geoblaze.d.ts
Outdated
export function identify( | ||
raster: GeoRaster, | ||
geom?: string | InputPoint | null | ||
): number[]; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
@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 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. |
Back to WIP. Finding the right structure for it to import the upstream georaster types |
As I've used more methods I've pushed some fixes. Feeling pretty tight. |
…ing index.js internally
type InputPolygon = number[][][] | Polygon | Feature<Polygon>; | ||
type InputPoint = number[] | Point | Feature<Point>; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch!
There was a problem hiding this 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"; |
There was a problem hiding this comment.
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 = { |
There was a problem hiding this comment.
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!
type InputPolygon = number[][][] | Polygon | Feature<Polygon>; | ||
type InputPoint = number[] | Point | Feature<Point>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch!
Hello, Is this PR active? I'm using typescript and this would be very useful for me |
Hello, my team would also greatly benefit from having these TS defs. I can also help out if need be. Thanks! |
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! |
Thank you very much! 🙏
…On Thu, 22 Jun 2023, 22:07 Rikki Schulte, ***@***.***> wrote:
I finished up & perfected 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!
—
Reply to this email directly, view it on GitHub
<#183 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACLNPQCFSSQV4UR4H6H4LOTXMSQW5ANCNFSM47L7YYNA>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
Thank you! |
Here you go: https://gist.github.com/StevenLangbroek/3de4e7e67cf49c764edf449da319bf69 I had to make a small tweak for our use-case; I named the |
Type of PR
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:
dist
folder for distribution. Webpack may be the tool to do it.types
in package.json to point to dist/geoblaze.d.tsgeoraster
package. Could be done as a follow-on for incremental progress.Note that I'm uncovering some JSDoc inaccuracies/inconsistencies in the process. I'll create a separate PR for them.