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 #59

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

Add Typescript definitions #59

wants to merge 22 commits into from

Conversation

twelch
Copy link

@twelch twelch commented Jun 27, 2021

Goal - add Typescript definitions.

Details:

  • Covers the public API for the top-level module only including the parseGeoraster() method and the Georaster` object it returns.
  • Ensure both default and named exports work
  • Adds a build step to copy to declaration file from src to dist folder.

Testing:

@twelch twelch marked this pull request as draft June 27, 2021 22:52
@twelch twelch marked this pull request as ready for review June 28, 2021 00:32
src/georaster.d.ts Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
@@ -43,7 +46,7 @@
"dependencies": {
"cross-fetch": "^3.0.4",
"georaster-to-canvas": "0.2.0",
"geotiff": "1.0.0-beta.13",
"geotiff": "file://../geotiff",
Copy link
Author

Choose a reason for hiding this comment

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

errant commit, this needs to be removed before merging

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.

Left some small comments. Awesome you included a test/types.ts!

/** defines the new raster image to generate as a window in the source raster image. Resolution (cell size) is determined from this */
export interface WindowOptions {
/** left side of the image window in pixel coordinates */
left: number
Copy link
Member

Choose a reason for hiding this comment

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

do these need commas or semicolons at the end of the lines?

* If the window options do not align exactly with the source image then a new
* one is generated using the resampleMethod. The best available overview will
* also be used if they are available. */
getValues?: (options: WindowOptions) => Promise<TypedArray[][]>;
Copy link
Member

Choose a reason for hiding this comment

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

I think it returns a 3D array like [band][row][column], so should be Promise<TypedArray[][][]>

* also be used if they are available. */
getValues?: (options: WindowOptions) => Promise<TypedArray[][]>;
/** experimental! returns a canvas picture of the data. */
toCanvas: (options: { height?: number; width?: number }) => ImageData
Copy link
Member

Choose a reason for hiding this comment

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

This function basically calls https://github.com/GeoTIFF/georaster-to-canvas/blob/master/index.js. It returns the canvas created by document.createElement("CANVAS"); here. Is there a way to specify an HTML Canvas Element with TypeScript?

toCanvas: (options: { height?: number; width?: number }) => ImageData
}

export type GeorasterMetadata = Pick<
Copy link
Member

Choose a reason for hiding this comment

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

I haven't used Pick before. So will this include xmax and ymin as optional arguments?

@DanielJDufour
Copy link
Member

Hey. I saw this nifty line in https://github.com/geotiffjs/geotiff.js/pull/253/files: npx [email protected] -y -- tsc --outdir dist-node/. I haven't seen something like this before, but I'm wondering, could we change "test-types": "tsc test/types.ts --noEmit" to "test-types": "npx --package=typescript -y -- tsc test/types.ts --noEmit" and thus avoid adding typescript as a dependency?

@jeafreezy
Copy link

Hi @DanielJDufour , thank you for this great library.

I am experiencing an issue with using this library with typescript, as it is a requirement for georaster for leaflet.

Error: Could not find a declaration file for module 'georaster'.

I would appreciate any solution to this. Thanks!

@jeafreezy
Copy link

Got rid of the error by creating a new file georaster.d.ts with the following content: declare module 'georaster';

@DanielJDufour
Copy link
Member

Hi, @jeafreezy . Great to hear you are making use of GeoRasterLayer. I think you might be able to solve your problems by creating a declarations.d.ts file and adding a line that says declare module "georaster";. In general you could put the declarations.d.ts file inside of a types folder inside of src. In any case, that is what we do with georaster-layer-for-leaflet for our other non-typescript dependencies. (see https://github.com/GeoTIFF/georaster-layer-for-leaflet/blob/master/src/types/declarations.d.ts). You might also need to set "typeRoots": ["types"] in the tsconfig.json file per https://github.com/GeoTIFF/georaster-layer-for-leaflet/blob/master/tsconfig.json.

I also wanted to apologize for taking so long with reviewing this PR. I want to say that adding types is definitely a priority for me. Unfortunately, I find myself trying to push a new major georaster upgrade across the goal line (improved accuracy and support for .asc, .jpg, .png, .prj, .wld). As soon as I'm done with that, I promise types will get more attention. Basically, I'm afraid if I lose focus on the new major georaster upgrade, it will be another several years till it happens. I hope that makes sense. And I really am sorry for the inconvenience and frustration this causes.

@jeafreezy
Copy link

Hi @DanielJDufour , thank you for the hint! It worked very well. Wish you the best in pushing the major upgrade!

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.

3 participants