geotiffjs / geotiff.js

geotiff.js is a small library to parse TIFF files for visualization or analysis. It is written in pure JavaScript, and is usable in both the browser and node.js applications.
https://geotiffjs.github.io/
MIT License
878 stars 183 forks source link

TypeScript: add `height` and `width` to return type of `readRasters()` #296

Closed MichaelBuhler closed 2 years ago

MichaelBuhler commented 2 years ago

Problem

According to the README (as quoted below), a call to readRasters() attaches height and width to the returned array.

Before this PR, the return type of readRasters() was Promise<TypedArray | TypedArray[]>, which does not include the height and width fields.

According to README.md

For convenience the result always has a width and height attribute:

const data = await image.readRasters();
const { width, height } = data;

Solution

Alter the return type of readRasters() to a new type named ReadRasterResult, which extends TypedArray | TypedArray[] to include { height: number; width: number }.

constantinius commented 2 years ago

Thanks @MichaelBuhler!

constantinius commented 2 years ago

@MichaelBuhler Unfortunately this messes with our docs generation: https://github.com/geotiffjs/geotiff.js/runs/5739344764?check_suite_focus=true#step:5:10

MichaelBuhler commented 2 years ago

Hmm. Sorry about that. I can look for a solution.

MichaelBuhler commented 2 years ago

This is related to https://github.com/jsdoc/jsdoc/issues/1285.

The types I wrote works in my IDE (WebStorm) but is not supported by JSDoc. I will investigate Closure Compiler type expressions.

MichaelBuhler commented 2 years ago

Well, I have been reading for an hour. This seems impossible to fix. JSDoc will never support TypeScript intersection types.

If we change the & to a |, the docs will generate, but TS won't like it. There is a JSDoc plugin called https://github.com/chriseaton/jsdoc-plugin-intersection which will convert the & to a | only during doc generation, but leave it in the source so that the generated TypeScript type is correct. @constantinius If you want me to implement this plugin in this repo, let me know.

We can revert this PR to fix the build. 😞

constantinius commented 2 years ago

@MichaelBuhler I see. Can we try the plugin? I'd rather not revert, as you already put so much effort into it.